-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Support Tail Based Sampling Processor From OTEL Collector Extension #5878
Support Tail Based Sampling Processor From OTEL Collector Extension #5878
Conversation
f9f80e0
to
9fd73c7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5878 +/- ##
==========================================
- Coverage 96.83% 96.82% -0.02%
==========================================
Files 342 342
Lines 16524 16525 +1
==========================================
- Hits 16001 16000 -1
- Misses 337 339 +2
Partials 186 186
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I tested the binary sizes
I would suggest we only include tail sampler. That's the component that's most useful in the final collector, which needs to be Jaeger. But the upstream load balancing collectors can be just OTEL Collectors. |
84a9b93
to
929c409
Compare
01c9743
to
e15af38
Compare
@@ -170,6 +170,10 @@ index-cleaner-integration-test: docker-images-elastic | |||
index-rollover-integration-test: docker-images-elastic | |||
$(MAKE) storage-integration-test COVEROUT=cover-index-rollover.out | |||
|
|||
.PHONY: tail-sampling-integration-test | |||
tail-sampling-integration-test: | |||
SAMPLING=tail $(MAKE) jaeger-v2-storage-integration-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this runs go test
, but when do you start the docker compose environment?
All other e2e tests have a driver script that orchestrates all components of the test, e.g. scripts/es-integration-test.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not using the docker-compose environment for my test. Calling e2eInitialize
is enough to start the Jaeger collector. You can simply run this test by calling make tail-sampling-integration-test
. Let me know if you want to change any of this setup though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. However, it means that the new docker compose file will begin to rot since it's not being exercised by the CI, something we tried to avoid (e.g. see e2e spm test). So it would be good to actually combine using docker compose with e2e test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see! What would you want this to look like? The current docker-compose set up generates load using tracegen which ideally we wouldn't want in the integration test so we can manually generate those. And the existing setup in the E2E tests does some nice things for us like flush the storage in between tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's why I was asking from the start what your plan would be. The way you are using e2e_integration framework is very lightweight, and I could easily see an alternative setup where everything is just orchestrated from a shell script
- run docker-compose with one config
- maybe don't include tracegen in compose, run it manually
- do a curl against query service to retrieve service names as JSON (trivial to write)
- shut down docker-compose (to clear the storage) and run again with different config
If you are interested to pursue this approach, I would suggest still merging this PR first so that we already have something in place. Can you finish the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro That sounds good to me and I can pursue that approach in a follow-up PR. And yes, working on the README now. Will push it up soon.
make sure to do |
Thank you so much for doing this for me! |
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
ad40036
to
91ae10c
Compare
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
@yurishkuro the README and the rest of the PR is ready for review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@yurishkuro - there looks to be a failing test in the CI. Is it a flaky test? I don't believe its related to my changes. |
🎉 🎉 🎉 |
Which problem is this PR solving?
Description of the changes
docker compose
to demonstrate usage of the tail-based sampling processor extension in jaeger.docker compose
setup describing the setup and usage of the new processorHow was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test