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

Update OTEL_EXPORTER_JAEGER_PROTOCOL parsing #2914

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Feb 18, 2022

Fixes #2910.

Changes

Align the OTEL_EXPORTER_JAEGER_PROTOCOL parsing with the spec

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

  • Appropriate CHANGELOG.md updated for non-trivial changes

@Kielek Kielek requested a review from a team February 18, 2022 06:36
@Kielek Kielek force-pushed the update-OTEL_EXPORTER_JAEGER_PROTOCOL-parsing branch 4 times, most recently from b2fe406 to 01b7bef Compare February 18, 2022 06:45
@Kielek Kielek force-pushed the update-OTEL_EXPORTER_JAEGER_PROTOCOL-parsing branch from 01b7bef to b5ff50e Compare February 18, 2022 06:55
Comment on lines 69 to 70
|`udp/thrift.compact`| Apache Thrift compact over UDP to a Jaeger Agent. |
|`http/thrift.binary`| Apache Thrift binary over HTTP to a Jaeger Collector. |
Copy link
Member

@pellared pellared Feb 18, 2022

Choose a reason for hiding this comment

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

I think this table is more about .NET API.

I would consider adding a footnote to the OTEL_EXPORTER_JAEGER_PROTOCOL or find another way to describe it in Environment Variables section. As for me it can be done like in https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol#environment-variables to remain consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed c1b1401

@pellared pellared changed the title Update otel exporter jaeger protocol parsing Update OTEL_EXPORTER_JAEGER_PROTOCOL parsing Feb 18, 2022
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #2914 (f210af0) into main (f234829) will increase coverage by 0.55%.
The diff coverage is 100.00%.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2914      +/-   ##
==========================================
+ Coverage   83.88%   84.43%   +0.55%     
==========================================
  Files         253      253              
  Lines        8910     8901       -9     
==========================================
+ Hits         7474     7516      +42     
+ Misses       1436     1385      -51     
Impacted Files Coverage Δ
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs 100.00% <100.00%> (+9.52%) ⬆️
...Exporter.Jaeger/JaegerExporterOptionsExtensions.cs 100.00% <100.00%> (ø)
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (ø)
...penTelemetry/Internal/EnvironmentVariableHelper.cs 80.00% <100.00%> (ø)
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
src/OpenTelemetry/Metrics/MetricReaderOptions.cs
...OpenTelemetry/Metrics/BaseExportingMetricReader.cs 85.00% <0.00%> (+1.66%) ⬆️
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 100.00% <0.00%> (+35.71%) ⬆️
... and 1 more


internal static class JaegerExporterOptionsExtensions
{
public static JaegerExportProtocol? ToJaegerExportProtocol(this string protocol) =>
Copy link
Member

Choose a reason for hiding this comment

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

subjective opinion (no need to be addressed)

As rule of thumb, I avoid adding extension methods for BCL classes unless these are very generic. This method is only used in JaegerExporterOptions and tests so I would rather not add is as an extension method to string. Personally, I would move it to JaegerExporterOptions class with following signature:

internal static JaegerExportProtocol? ParseProtocol(string value)

or

internal static bool ParseProtocol(string value, out JaegerExportProtocol result)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, I agree, but the same convention is for parsing OTLP parsing value. I would keep it as it is.

Copy link
Member

Choose a reason for hiding this comment

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

+1, echo from #2875 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2919 created

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I left a few comments, but none of them is a blocker for me

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.

@pellared pellared requested a review from CodeBlanch February 21, 2022 06:51
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

@CodeBlanch CodeBlanch merged commit 4b3ee96 into open-telemetry:main Feb 22, 2022
@Kielek Kielek deleted the update-OTEL_EXPORTER_JAEGER_PROTOCOL-parsing branch February 22, 2022 19:16
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 OTEL_EXPORTER_JAEGER_PROTOCOL parsing
5 participants