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

[Traces + Logs] Clean up repo exporters to use new DI patterns #3640

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 8, 2022

Changes

Updates the trace & logging exporters to use the new DI API surface.

OtlpLogExporter got a couple features with this upgrade:

  • Now obtains its options instance through IOptions so it can be bound to IConfiguration
  • Now calls the TryEnableIHttpClientFactoryIntegration helper so it should also be able to use HttpClientFactory integration

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes

@CodeBlanch CodeBlanch requested a review from a team as a code owner September 8, 2022 18:03
Comment on lines +76 to +79
// TODO: exporterOptions.BatchExportProcessorOptions is
// BatchExportActivityProcessorOptions which is using tracing
// environment variables. There should probably be a dedicated
// setting for logs using BatchExportLogRecordProcessorOptions
Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest You want me to spin up an issue for this?

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #3640 (f6c5980) into main (5208ec0) will increase coverage by 0.00%.
The diff coverage is 94.73%.

❗ Current head f6c5980 differs from pull request most recent head c3e98d3. Consider uploading reports for the commit c3e98d3 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3640   +/-   ##
=======================================
  Coverage   87.35%   87.35%           
=======================================
  Files         282      282           
  Lines       10132    10142   +10     
=======================================
+ Hits         8851     8860    +9     
- Misses       1281     1282    +1     
Impacted Files Coverage Δ
...ryProtocol.Logs/OtlpLogExporterHelperExtensions.cs 90.47% <83.33%> (-1.84%) ⬇️
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs 97.67% <100.00%> (+2.32%) ⬆️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs 90.90% <100.00%> (+5.19%) ⬆️
....Exporter.Zipkin/ZipkinExporterHelperExtensions.cs 97.61% <100.00%> (+2.49%) ⬆️
...Propagators/OpenTelemetryPropagatorsEventSource.cs 87.50% <0.00%> (-12.50%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 73.52% <0.00%> (-5.89%) ⬇️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 86.66% <0.00%> (-2.23%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 95.05% <0.00%> (-1.10%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️
... and 2 more

@CodeBlanch CodeBlanch merged commit fb6463d into open-telemetry:main Sep 8, 2022
@CodeBlanch CodeBlanch deleted the exporter-cleanup branch September 8, 2022 19:08
@TimothyMothra
Copy link
Contributor

Will the same change come to metrics?

InMemoryExporterMetricsExtensions also uses the IDeferredMeterProviderBuilder pattern.
I wrote tests for this in the OpenTelemetry.Extensions.Hosting.Tests project: InMemoryExporterMetricsExtensionsTests.

@CodeBlanch
Copy link
Member Author

@TimothyMothra

Will the same change come to metrics?

Totally! My goal is for all three signals to have the same (~) API for build-up. I'll start on metrics after a couple of these clean-up PRs for traces + logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants