-
Notifications
You must be signed in to change notification settings - Fork 45
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
editoast: setup tracing with opentelemetry for TestApp
#10122
editoast: setup tracing with opentelemetry for TestApp
#10122
Conversation
TestApp
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10122 +/- ##
==========================================
+ Coverage 79.93% 81.44% +1.50%
==========================================
Files 1057 1058 +1
Lines 106302 104318 -1984
Branches 724 724
==========================================
- Hits 84977 84966 -11
+ Misses 21283 19310 -1973
Partials 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c5b5f96
to
8a7a2ad
Compare
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.
Thank you for the PR. Let's see if we can improve it.
8a7a2ad
to
c361ea8
Compare
c361ea8
to
b5ff13b
Compare
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 for this PR. A few comments about architecture and test isolation. Besides, with the context of what you're working on I suppose this is about collecting logs for unit testing, but it's not clear to me how that would work. Why does printing logs on stdout help? Do you plan to capture the stream? A bit more context in the PR description or in the related issue would help.
Apologies for that. I've added more details in the description, and I hope it helps clarify things. |
Thanks for adding some context. So if I get it right, the point is not to necessarily print on stdout, but rather to setup OpenTelemetry so that the request header can be read. The fact that it prints on stdout is irrelevant, or am I missing smth? I'm surely lacking OpenTelemetry and tracing knowledge, but I still don't understand what setting up a span exporter has to do with reading the HTTP request headers? Especially since the middleware |
You're right, printing to |
@leovalais From what I understand, But as said by @hamz2a, one of the main idea of this PR is to have a similar setup of the subscriber for tests and for production code. We had a hard time debugging what was not working and part of it was that it was working well with a |
b5ff13b
to
4492eb8
Compare
4492eb8
to
112a5c5
Compare
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 believe it's in a much better state now 🚀. Thank you 🎉
9d9e553
to
373cf12
Compare
373cf12
to
45217d4
Compare
033cbc3
to
5c49d18
Compare
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.
Almost there, thanks for the changes! There's still a dependency order issue, but apart from that, lgtm.
Yes, but struct TracingConfig {
stream: enum { Stderr, Stdout },
telemetry: Option<struct {
service_name: String,
telemetry_endpoint: Url
}>
} Wdyt? |
5c49d18
to
4418d21
Compare
4418d21
to
72ae856
Compare
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 for all the modifications!
Signed-off-by: hamz2a <[email protected]>
72ae856
to
52acc68
Compare
closes #10123
I added the
OpenTelemetry
layer inTestApp
to capture thetrace_id
set in the header, so we can test the endpoint we'll add in the ticket.init_tracing
to accept a customSpanExporter
for better flexibility, including support for testing scenarios.main.rs
to configure and initialize tracing.test_app.rs
to use opentelemetry layer.