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

Fix the default endpoint for OTLP exporter when using http/protobuf protocol #2868

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Feb 7, 2022

Fixes #2857.

Changes

Default endpoint for OTLP exporter based on chosen protocol.

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

  • Appropriate CHANGELOG.md updated for non-trivial changes
    * [ ] Design discussion issue #
    * [ ] Changes in public API reviewed

@Kielek Kielek requested a review from a team February 7, 2022 11:44
@Kielek Kielek changed the title default endpoint for otel exported - http/protobuf default endpoint for otel exporter - http/protobuf Feb 7, 2022
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.

LGTM

@Kielek Kielek force-pushed the otlp-exporter-over-http-protobuf-fixed-endpoint branch from caa0da7 to 2de961a Compare February 7, 2022 11:52
@pellared pellared changed the title default endpoint for otel exporter - http/protobuf Fix the default endpoint for OTLP exporter when using http/protobuf protocol Feb 7, 2022
@Kielek Kielek force-pushed the otlp-exporter-over-http-protobuf-fixed-endpoint branch from 2de961a to d46cf5c Compare February 7, 2022 11:55
@Kielek Kielek force-pushed the otlp-exporter-over-http-protobuf-fixed-endpoint branch 2 times, most recently from 5705723 to 9cf5244 Compare February 7, 2022 11:59
@Kielek Kielek force-pushed the otlp-exporter-over-http-protobuf-fixed-endpoint branch from 9cf5244 to a003656 Compare February 7, 2022 12:16
@Kielek Kielek requested a review from pellared February 7, 2022 12:40
@Kielek Kielek force-pushed the otlp-exporter-over-http-protobuf-fixed-endpoint branch from a003656 to 2edca30 Compare February 7, 2022 12:41
@Kielek Kielek force-pushed the otlp-exporter-over-http-protobuf-fixed-endpoint branch from 5c7bc1d to 2f9beb3 Compare February 7, 2022 13:24
@pellared pellared requested a review from CodeBlanch February 7, 2022 17:53
@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #2868 (ee14574) into main (84821ff) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2868      +/-   ##
==========================================
+ Coverage   84.22%   84.23%   +0.01%     
==========================================
  Files         251      251              
  Lines        8883     8889       +6     
==========================================
+ Hits         7482     7488       +6     
  Misses       1401     1401              
Impacted Files Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (ø)
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 97.80% <100.00%> (ø)
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 100.00% <100.00%> (ø)
...metryProtocol/OtlpTraceExporterHelperExtensions.cs 94.73% <100.00%> (-0.27%) ⬇️

@Kielek
Copy link
Contributor Author

Kielek commented Feb 8, 2022

@CodeBlanch, I see that Compatibility API check is failing
with following errors

C:\Users\runneradmin\.nuget\packages\microsoft.dotnet.apicompat\6.0.0-beta.21308.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveAttribute : Attribute 'System.Runtime.CompilerServices.CompilerGeneratedAttribute' exists on 'OpenTelemetry.Exporter.OtlpExporterOptions.Endpoint.get()' in the contract but not the implementation. [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj]
C:\Users\runneradmin\.nuget\packages\microsoft.dotnet.apicompat\6.0.0-beta.21308.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveAttribute : Attribute 'System.Runtime.CompilerServices.CompilerGeneratedAttribute' exists on 'OpenTelemetry.Exporter.OtlpExporterOptions.Endpoint.set(System.Uri)' in the contract but not the implementation. [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj]
C:\Users\runneradmin\.nuget\packages\microsoft.dotnet.apicompat\6.0.0-beta.21308.1\build\Microsoft.DotNet.ApiCompat.targets(96,5): error : ApiCompat failed for 'D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\bin\Release\netstandard2.1\OpenTelemetry.Exporter.OpenTelemetryProtocol.dll' [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj]
C:\Users\runneradmin\.nuget\packages\microsoft.dotnet.apicompat\6.0.0-beta.21308.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveAttribute : Attribute 'System.Runtime.CompilerServices.CompilerGeneratedAttribute' exists on 'OpenTelemetry.Exporter.OtlpExporterOptions.Endpoint.get()' in the contract but not the implementation. [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj]
C:\Users\runneradmin\.nuget\packages\microsoft.dotnet.apicompat\6.0.0-beta.21308.1\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveAttribute : Attribute 'System.Runtime.CompilerServices.CompilerGeneratedAttribute' exists on 'OpenTelemetry.Exporter.OtlpExporterOptions.Endpoint.set(System.Uri)' in the contract but not the implementation. [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj]
C:\Users\runneradmin\.nuget\packages\microsoft.dotnet.apicompat\6.0.0-beta.21308.1\build\Microsoft.DotNet.ApiCompat.targets(96,5): error : ApiCompat failed for 'D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\bin\Release\netstandard2.0\OpenTelemetry.Exporter.OpenTelemetryProtocol.dll' [D:\a\opentelemetry-dotnet\opentelemetry-

As I understand the issue is related to Microsoft.DotNet.ApiCompat tool.
The second tool using PublicApi.Shipped.txt and PublicApi.Unshipped.txt is passing correctly. Do you have any advice how to handle such issue?

@CodeBlanch
Copy link
Member

@Kielek

I see that Compatibility API check is failing

No idea. It seems like an issue because we are going from an auto-property to one with implementation.

Try updating this version:

<PackageReference Include="Microsoft.DotNet.ApiCompat" Version="6.0.0-beta.21308.1" PrivateAssets="All" />

To 6.0.0-beta.22107.2. That is the latest 6.0.0 version. There are some newer 7.0.0 ones: https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/1a5f89f6-d8da-4080-b15f-242650c914a8/nuget/v3/flat2/microsoft.dotnet.apicompat/index.json

Maybe we will get lucky and it is an issue that has already been fixed.

@cijothomas Might need some help on this.

@utpilla
Copy link
Contributor

utpilla commented Feb 9, 2022

@Kielek For the APICompat issue, could you please run the below command locally at the root of the repo?

dotnet build /p:CheckAPICompatibility=true,BaselineAllAPICompatError=true --configuration Release

This should create a file named ApiCompatBaseline.txt under src/OpenTelemetry.Exporter.OpenTelemetryProtocol/. Please add this file and commit the changes.

in OpenTelemetry.Exporter.OpenTelemetryProtocol
@Kielek Kielek requested review from alanwest and pellared February 9, 2022 08:22
@Kielek
Copy link
Contributor Author

Kielek commented Feb 9, 2022

@Kielek For the APICompat issue, could you please run the below command locally at the root of the repo?

dotnet build /p:CheckAPICompatibility=true,BaselineAllAPICompatError=true --configuration Release

This should create a file named ApiCompatBaseline.txt under src/OpenTelemetry.Exporter.OpenTelemetryProtocol/. Please add this file and commit the changes.

@utpilla do you consider this as a workaround or good solution? In my opinion the changes should not be detected as breaking change.

File commited.

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.

This looks more readable and maintainable 👍

@utpilla
Copy link
Contributor

utpilla commented Feb 9, 2022

@utpilla do you consider this as a workaround or good solution? In my opinion the changes should not be detected as breaking change.

File commited.

This is the defined way of suppressing any error if you feel that you want to ignore it. I'm not sure if this should be detected as a breaking change or not. I'll provide the feedback to the ApiCompat team about this.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @Kielek!

@pellared pellared requested a review from cijothomas February 14, 2022 08:42
@cijothomas
Copy link
Member

Merging now, as this should be part of the 1.2 stable. Has enough approvals. and waited 2+days.

@cijothomas cijothomas merged commit 73b99b6 into open-telemetry:main Feb 15, 2022
@Kielek Kielek deleted the otlp-exporter-over-http-protobuf-fixed-endpoint branch February 16, 2022 05:30
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.

OTLP/HTTP exporters should default to port 4318
6 participants