Skip to content
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] Update README for UseOtlpExporter & new experimental flag #5452

Merged
merged 17 commits into from
Mar 21, 2024

Conversation

CodeBlanch
Copy link
Member

Relates to #5400
Relates to #5435

Changes

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)

@CodeBlanch CodeBlanch added pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package documentation Documentation related labels Mar 14, 2024
@CodeBlanch CodeBlanch requested a review from a team March 14, 2024 22:25
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.46%. Comparing base (6250307) to head (8e2e25a).
Report is 136 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5452      +/-   ##
==========================================
+ Coverage   83.38%   85.46%   +2.08%     
==========================================
  Files         297      289       -8     
  Lines       12531    12463      -68     
==========================================
+ Hits        10449    10652     +203     
+ Misses       2082     1811     -271     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 85.25% <100.00%> (?)
unittests-Solution-Stable 85.40% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...lder/OpenTelemetryBuilderOtlpExporterExtensions.cs 100.00% <100.00%> (ø)

... and 63 files with indirect coverage changes

The `UseOtlpExporter` has the following behaviors:

* Calling `UseOtlpExporter` automatically enables logging, metrics, and tracing.
Calls to `WithLogging`, `WithTracing`, and/or `WithLogging` are NOT required
Copy link
Contributor

@utpilla utpilla Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds a little confusing. We start off saying calls to the "With" extension methods are not required and then go on to say however only the telemetry enabled using "With" extension methods would be exported.

Maybe update this to something like:

When calling UseOtlpExporter method, you don't need to separately call AddOtlpExporter method for WithLogging, WithTracing, and/or WithMetrics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of tricky. The docs we have that I could find seem to all show "Sdk.Create*" style which really doesn't help for this cross-cutting extension using the hosting style. I decided to show some real examples of how to do it. LMK what you think.

Copy link
Member

@vishweshbankwar vishweshbankwar Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithLogging - This is not a stable way of configuring logging today so I think we should not mention it.

To keep things simple, what do you guys think about something like this.

Using UseOtlpExporter automatically enables the OTLP exporter for logs, metrics, and traces. 
For additional sdk configuration, like adding meters to the MeterProvider, 
incorporating sources to the TracerProvider, or 
configuring resources for all three types of signals, additional setup will required. 
For more information, refer to the details below. (Followed by what we have below)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the mention of the "With" calls and simplified it down to just...

Calling UseOtlpExporter automatically enables logging, metrics, and tracing however only telemetry which has been enabled will be exported.

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments. LGTM overall.


* All signals

* `OTEL_DOTNET_EXPERIMENTAL_OTLP_ENABLE_INMEMORY_RETRY`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishweshbankwar I have questions regarding the name - does it mean later there will be something like "OTLP_ENABLE_ONDISK_RETRY? What's the thinking which leads to the current name instead of OTEL_DOTNET_EXPERIMENTAL_OTLP_ENABLE_RETRY`?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it mean later there will be something like "OTLP_ENABLE_ONDISK_RETRY

That was the thinking. Wanted to have decoupled experimental environment variables.

When we move to stable, I would like it to be OTEL_DOTNET_OTLP_ENABLE_RETRY and define possible values for it like inmemory and disk.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with couple non-blocking comments.

@CodeBlanch CodeBlanch merged commit 3ff1df2 into open-telemetry:main Mar 21, 2024
33 checks passed
@CodeBlanch CodeBlanch deleted the otlp-exporter-doc-updates branch March 21, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants