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

[Exporter.Geneva] Add named options support for GenevaTraceExporter and GenevaMetricExporter #1218

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Jun 3, 2023

Changes

  • Add named options support for GenevaTraceExporter and GenevaMetricExporter
  • Add a new overload for AddGenevaMetricExporter without any parameters to avoid warning RS0026

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@utpilla utpilla requested a review from a team as a code owner June 3, 2023 01:07
@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Jun 3, 2023
@codecov
Copy link

codecov bot commented Jun 3, 2023

Codecov Report

Merging #1218 (4a96290) into main (18b0b03) will increase coverage by 0.90%.
The diff coverage is 96.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1218      +/-   ##
==========================================
+ Coverage   73.17%   74.07%   +0.90%     
==========================================
  Files         260      265       +5     
  Lines        9133     9347     +214     
==========================================
+ Hits         6683     6924     +241     
+ Misses       2450     2423      -27     
Impacted Files Coverage Δ
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 93.33% <ø> (ø)
...etry.Exporter.Geneva/TLDExporter/TldLogExporter.cs 37.11% <ø> (ø)
src/OpenTelemetry.Sampler.AWS/Clock.cs 100.00% <ø> (ø)
...rs.AWS/Http/ServerCertificateValidationProvider.cs 59.67% <80.00%> (+0.30%) ⬆️
src/OpenTelemetry.Sampler.AWS/FallbackSampler.cs 88.88% <83.33%> (-11.12%) ⬇️
....Exporter.Geneva/GenevaExporterHelperExtensions.cs 68.18% <84.61%> (+12.62%) ⬆️
.../OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs 82.60% <91.30%> (+10.86%) ⬆️
...r.Geneva/Metrics/GenevaMetricExporterExtensions.cs 93.33% <92.30%> (+3.33%) ⬆️
src/OpenTelemetry.Sampler.AWS/RulesCache.cs 90.90% <96.00%> (+17.83%) ⬆️
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 92.94% <100.00%> (+0.25%) ⬆️
... and 13 more

... and 4 files with indirect coverage changes


if (configure != null)
{
builder.ConfigureServices(services => services.Configure(name, configure));
Copy link
Member

Choose a reason for hiding this comment

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

FYI this will work fine today for metrics & traces because they have different options classes. Logging appears to be sharing GenevaExporterOptions with tracing so if/when logging gets hooked up to DI/options there will be issues with the delegates potentially clobbering each other. Cross that bridge when we come to it 😄

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants