-
Notifications
You must be signed in to change notification settings - Fork 32
π¨β¨ Implement tracing sampling strategy (π§ devops π§) #8421
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
π¨β¨ Implement tracing sampling strategy (π§ devops π§) #8421
Conversation
Codecov Reportβ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8421 +/- ##
==========================================
+ Coverage 87.32% 87.63% +0.30%
==========================================
Files 1877 1999 +122
Lines 73118 77828 +4710
Branches 1333 1338 +5
==========================================
+ Hits 63850 68201 +4351
- Misses 8869 9227 +358
- Partials 399 400 +1
Continue to review full report in Codecov by Sentry.
π New features to boost your workflow:
|
sanderegg
left a comment
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.
you should:
- add the Red lamp to your PR description since it seems you wanna test stuff
- create MRs in the osparc-config repos concerning your ENV var
pcrespov
left a comment
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.
thx
π§ͺ CI InsightsHere's what we observed from your CI run for 009d00c. π’ All jobs passed!But CI Insights is watching π |
matusdrobuliak66
left a comment
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.
π
GitHK
left a comment
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.
thanks
pcrespov
left a comment
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.
thx for the effort!
mrnicegyu11
left a comment
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.
Very nice, left some comments but most are minor or questions. Thanks a lot for this big contribution π
services/web/server/src/simcore_service_webserver/application.py
Outdated
Show resolved
Hide resolved
|
@Mergifyio queue |
π Waiting for conditions to match
|
|



N.B.
I rerequested reviews from everyone because I had to do quite a lot of changes to how we configure the tracing.
What do these changes do?
TraceProviderwe use from the opentelemetry library is global, which poses a limitation in our tests. Hence, I started using a localTraceProviderwhich is passed around everywhere. This is not supported by the aiohttp server instrumentation lib (Allow passing aTracerProviderwhen instrumenting an aiohttp serverΒ open-telemetry/opentelemetry-python-contrib#3801) so I had to fix the middleware from that lib.Related issue/s
TracerProviderwhen instrumenting an aiohttp serverΒ open-telemetry/opentelemetry-python-contrib#3801How to test
Dev-ops
TRACING_SAMPLING_PROBABILITYenv var to all repo.config files.TRACING_SAMPLING_PROBABILITYnow should also configure the tracing sampling strategy on ops traefik (similar to here). This PR should also ensure thatTRACING_OPENTELEMETRY_COLLECTOR_SAMPLING_PERCENTAGEis not required anywhere (as it has now been deleted from therepo.configfiles. This has been added here: Add tracing sampling percentageΒ osparc-ops-environments#1230