-
Notifications
You must be signed in to change notification settings - Fork 450
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
OTLP Example fixes #2394
OTLP Example fixes #2394
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2394 +/- ##
=====================================
Coverage 79.4% 79.4%
=====================================
Files 123 123
Lines 21485 21485
=====================================
Hits 17068 17068
Misses 4417 4417 ☔ View full report in Codecov by Sentry. |
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 the fixes. Some nit comments.
Collector](https://github.com/open-telemetry/opentelemetry-collector) via OTLP | ||
over selected protocol such as HTTP/protobuf or HTTP/json. The Collector then sends the data to the appropriate | ||
backend, in this case, the logging Exporter, which displays data to console. | ||
over HTTP (using `protobuf` encoding by default but can be changed to use |
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.
also mention using reqwest
client by default.
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.
omitted it intentionally, as I expect request-blocking will be the default when we make BatchProcessor with thread as default in sdk. Will make the changes as sdk changes are done.
); | ||
|
||
let tracer_provider = result.unwrap(); | ||
let tracer_provider = init_traces()?; | ||
global::set_tracer_provider(tracer_provider.clone()); |
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.
It might be better to set the global provider within the init_traces
and init_metrics
method itself. That could better convey to the users that setting global providers is required as part of the initialization setup.
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 wanted to show explicitly that it is cheap and okay to clone the provider here.
Maybe setup another help create_provider, which is invoked from init_traces so we can cover your recommendation, and also show the cloning aspect..?
Fixes a few small, but important things
tracing::fmt
layer along with above. This demonstrates how one can view OTel's own internal logs in stdout usingtracing::fmt
. We intend to eliminate the self-diagnostics example, as that is now redundant. (The actual removal is in a future PR, after making sure we have everything covered.)(Some of these are in prep to land #2386 )