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
woshilapin
left a comment
There was a problem hiding this comment.
Thank you for the PR. Let's see if we can improve it.
8a7a2ad to
c361ea8
Compare
c361ea8 to
b5ff13b
Compare
leovalais
left a comment
There was a problem hiding this comment.
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
woshilapin
left a comment
There was a problem hiding this comment.
I believe it's in a much better state now 🚀. Thank you 🎉
9d9e553 to
373cf12
Compare
373cf12 to
45217d4
Compare
033cbc3 to
5c49d18
Compare
leovalais
left a comment
There was a problem hiding this comment.
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
leovalais
left a comment
There was a problem hiding this comment.
LGTM, thanks for all the modifications!
Signed-off-by: hamz2a <atrari.hamza@gmail.com>
72ae856 to
52acc68
Compare
closes #10123
I added the
OpenTelemetrylayer inTestAppto capture thetrace_idset in the header, so we can test the endpoint we'll add in the ticket.init_tracingto accept a customSpanExporterfor better flexibility, including support for testing scenarios.main.rsto configure and initialize tracing.test_app.rsto use opentelemetry layer.