Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v2] Use health check in grpc e2e test #6113

Merged
merged 9 commits into from
Oct 29, 2024
Merged

[v2] Use health check in grpc e2e test #6113

merged 9 commits into from
Oct 29, 2024

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Oct 22, 2024 •
edited
Loading

Which problem is this PR solving?

Description of the changes

@yurishkuro
[v2] Use health check in grpc e2e test
2879553 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro mentioned this pull request Oct 22, 2024
Closed
2 tasks
@yurishkuro
cleanup
f05b569 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro added changelog:test Change that's adding missing tests or correcting existing tests changelog:ci Change related to continuous integration / testing and removed changelog:test Change that's adding missing tests or correcting existing tests labels Oct 22, 2024
@yurishkuro
cleanup
ce35234 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
comments
27bb55c 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@codecov Codecov
Copy link

codecov bot commented Oct 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.41%. Comparing base (e327edb) to head (652eb82).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6113      +/-   ##
==========================================
+ Coverage   96.18%   96.41%   +0.23%     
==========================================
  Files         353      353              
  Lines       20135    20135              
==========================================
+ Hits        19366    19414      +48     
+ Misses        586      535      -51     
- Partials      183      186       +3
Flag Coverage Δ
badger_v1 8.32% <ø> (?)
badger_v2 1.68% <ø> (ø)
cassandra-4.x-v1 14.39% <ø> (ø)
cassandra-4.x-v2 1.62% <ø> (ø)
cassandra-5.x-v1 14.39% <ø> (ø)
cassandra-5.x-v2 1.62% <ø> (ø)
elasticsearch-6.x-v1 18.50% <ø> (-0.01%) ⬇️
elasticsearch-7.x-v1 18.58% <ø> (-0.01%) ⬇️
elasticsearch-8.x-v1 18.76% <ø> (ø)
elasticsearch-8.x-v2 1.67% <ø> (-0.02%) ⬇️
grpc_v1 9.52% <ø> (-0.01%) ⬇️
grpc_v2 7.00% <ø> (-0.01%) ⬇️
kafka-v1 8.88% <ø> (?)
kafka-v2 1.68% <ø> (ø)
memory_v2 1.68% <ø> (ø)
opensearch-1.x-v1 18.63% <ø> (-0.01%) ⬇️
opensearch-2.x-v1 18.64% <ø> (+<0.01%) ⬆️
opensearch-2.x-v2 1.68% <ø> (+0.01%) ⬆️
tailsampling-processor 0.47% <ø> (ø)
unittests 95.33% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mahadzaryab1 mahadzaryab1 mentioned this pull request Oct 26, 2024
Merged
4 tasks
yurishkuro pushed a commit that referenced this pull request Oct 27, 2024
@mahadzaryab1
[fix][grpc] Disable tracing in grpc storage writer clients (#6125)
a4efe57 
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->

## Which problem is this PR solving?
- Fixes #5971 
- Towards #6113 and #5859

## Description of the changes
- This PR fixes an issue where the GRPC remote storage client was
provided a tracer which was resulting in an infinite loop of trace
generation. This infinite loop would happen when we would try to write a
trace to storage which would generate a new trace that needed to be
written and so on. This PR provides a fix for this by using a noop
tracer for the writer clients so that we do not generate traces on the
write paths but still do so when reading.
- This is likely just a temporary fix and we'll want to monitor
open-telemetry/opentelemetry-collector#10663
for a better long-term fix.

## How was this change tested?
- Added the healthcheck endpoint which was previously failing in #6113.

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

## Co-Authors 
This PR is a continuation of
#5979
Co-authored-by: cx <1249843194@qq.com>

---------

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@mahadzaryab1
Copy link
Collaborator

@yurishkuro this should be unblocked now

@yurishkuro
Copy link
Member Author

please stamp

@yurishkuro yurishkuro marked this pull request as ready for review October 29, 2024 02:57
@yurishkuro yurishkuro requested a review from a team as a code owner October 29, 2024 02:57
mahadzaryab1
mahadzaryab1 approved these changes Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one comment otherwise lgtm

cmd/jaeger/internal/integration/e2e_integration.go Outdated
return false
}

// @nocommit: this is a temporary hack to ensure reproducible failure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this bit now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, but it was supposed to be failing CI, something's not right

@yurishkuro
fix-nocommit
d97a80f 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
fix-nocommit
f885518 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
fix-nocommit
fc71e5d 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
fix-nocommit
652eb82 
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro merged commit 3c1e85d into jaegertracing:main Oct 29, 2024
49 checks passed
@yurishkuro yurishkuro deleted the fix-5859 branch October 29, 2024 14:54
yurishkuro added a commit to yurishkuro/jaeger that referenced this pull request Nov 2, 2024
@yurishkuro
Revert "[v2] Use health check in grpc e2e test (jaegertracing#6113)"
bb84dd5 
This reverts commit 3c1e85d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahadzaryab1 mahadzaryab1 mahadzaryab1 approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay pavolloffay is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
changelog:ci Change related to continuous integration / testing v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[v2] Use health check extension in e2e test instead of /metrics
2 participants
@yurishkuro @mahadzaryab1