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

Added ServiceName option to OtlpExporter #1420

Merged
merged 6 commits into from
Oct 30, 2020

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Oct 29, 2020

Fixes #1416

Changes

Add ServiceName option to OtlpExporter to bring it into parity with Zipkin & Jaeger.

TODOs:

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

@CodeBlanch CodeBlanch requested a review from a team October 29, 2020 17:47
@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #1420 into master will increase coverage by 0.56%.
The diff coverage is 67.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1420      +/-   ##
==========================================
+ Coverage   81.31%   81.87%   +0.56%     
==========================================
  Files         227      227              
  Lines        6117     6086      -31     
==========================================
+ Hits         4974     4983       +9     
+ Misses       1143     1103      -40     
Impacted Files Coverage Δ
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <47.36%> (+13.06%) ⬆️
...try.Exporter.OpenTelemetryProtocol/OtlpExporter.cs 58.97% <85.00%> (+30.40%) ⬆️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (ø)
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
....Prometheus/PrometheusExporterMetricsHttpServer.cs 74.50% <0.00%> (-1.97%) ⬇️

@CodeBlanch
Copy link
Member Author

@eddynaka @cijothomas Would you guys mind taking another look at this? In order to make the tests work I had to switch the resource from a static on ActivityExtensions to a field on OtlpExporter.

@eddynaka
Copy link
Contributor

@eddynaka @cijothomas Would you guys mind taking another look at this? In order to make the tests work I had to switch the resource from a static on ActivityExtensions to a field on OtlpExporter.

@CodeBlanch , took a look. Looks fine. Don't think that will affect performance/allocation much.
Very nice.

@cijothomas cijothomas merged commit be5a7a7 into open-telemetry:master Oct 30, 2020
@CodeBlanch CodeBlanch deleted the otlp-servicename-option branch October 31, 2020 16:47
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.

Add ServiceName property to OtlpExporterOptions
3 participants