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

Change the default port that the OTLP exporters point to. #2113

Merged

Conversation

jkwatson
Copy link
Contributor

Resolves #2110

@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #2113 (3d68ff9) into master (4897f4c) will increase coverage by 85.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2113       +/-   ##
=============================================
+ Coverage          0   85.33%   +85.33%     
- Complexity        0     2117     +2117     
=============================================
  Files             0      241      +241     
  Lines             0     8086     +8086     
  Branches          0      893      +893     
=============================================
+ Hits              0     6900     +6900     
- Misses            0      854      +854     
- Partials          0      332      +332     
Impacted Files Coverage Δ Complexity Δ
...elemetry/exporter/otlp/OtlpGrpcMetricExporter.java 53.93% <ø> (ø) 6.00 <0.00> (?)
...ntelemetry/exporter/otlp/OtlpGrpcSpanExporter.java 76.14% <100.00%> (ø) 5.00 <1.00> (?)
...ava/io/opentelemetry/api/common/AttributeType.java 100.00% <0.00%> (ø) 1.00% <0.00%> (?%)
.../java/io/opentelemetry/context/ContextStorage.java 50.00% <0.00%> (ø) 2.00% <0.00%> (?%)
...va/io/opentelemetry/context/DefaultContextKey.java 75.00% <0.00%> (ø) 1.00% <0.00%> (?%)
...io/opentelemetry/sdk/metrics/MeterSdkProvider.java 95.00% <0.00%> (ø) 8.00% <0.00%> (?%)
...ava/io/opentelemetry/api/common/LabelsBuilder.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...ava/io/opentelemetry/opentracingshim/SpanShim.java 73.61% <0.00%> (ø) 26.00% <0.00%> (?%)
.../metrics/aggregator/DoubleLastValueAggregator.java 100.00% <0.00%> (ø) 7.00% <0.00%> (?%)
.../opentelemetry/context/ContextExecutorService.java 76.92% <0.00%> (ø) 13.00% <0.00%> (?%)
... and 233 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4897f4c...3d68ff9. Read the comment docs.

@@ -111,8 +112,7 @@ private OtlpGrpcSpanExporter(ManagedChannel channel, long deadlineMs) {
*/
@Override
public CompletableResultCode export(Collection<SpanData> spans) {
spansSeen.add(
spans.size(), Labels.of("exporter", OtlpGrpcMetricExporter.class.getSimpleName()));
spansSeen.add(spans.size(), Labels.of("exporter", OtlpGrpcSpanExporter.class.getSimpleName()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fixes! While we're here, should we extract constants for constant labels?

Copy link
Contributor

Choose a reason for hiding this comment

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

Though as an aside, after seeing some metrics code recently, I sort of wish constant labels could be specified when creating a meter instead of only when observing to save the tedium of always extracting constants to save allocation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a feature in the API that we decided to remove from the API for now. It may very well come back later, but we wanted to limit the features pre-GA, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I guess this could just be a constant for the labels instance, eh? Very happy to make that happen, either as a follow-up, or part of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either's fine with me this is approved :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

easy enough to do, so I just amended this PR with the constant label extraction

@jkwatson jkwatson force-pushed the change_collector_default_port branch from b03b3fe to 3d68ff9 Compare November 23, 2020 18:26
@jkwatson jkwatson requested a review from Oberon00 as a code owner November 23, 2020 18:26
@jkwatson jkwatson merged commit e599d0e into open-telemetry:master Nov 23, 2020
@jkwatson jkwatson deleted the change_collector_default_port branch November 23, 2020 18: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.

Update the default port for OTLP exporters according to spec
2 participants