-
Notifications
You must be signed in to change notification settings - Fork 775
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
Add HTTP protocol test case to OTLP exporter integration tests #2692
Add HTTP protocol test case to OTLP exporter integration tests #2692
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2692 +/- ##
==========================================
- Coverage 84.06% 84.03% -0.03%
==========================================
Files 251 251
Lines 8778 8778
==========================================
- Hits 7379 7377 -2
- Misses 1399 1401 +2
|
I don't see it either so it might be the collector :/ |
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/IntegrationTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/IntegrationTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, perhaps we want to rename CollectorEndpoint
to CollectorHostname
/CollectorHostName
.
Agree. The main problem is.... That the test for HttpProtobuf are failing and the reason is unknown. I do not know when I will have time to investigate it further |
Is this a priority? I don't know much history about this component, why would people want to use HTTP instead of gRPC? I can imagine the following:
|
I heard that it is also more performant and it is the default proposed by spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md. I think the URL for HttpProtobuf is bad in the test code. I will check it on Monday |
@tomkerkhove There were two issues:
Both issues were addressed here 75ec65f |
@open-telemetry/dotnet-approvers PTAL |
Thanks for sharing and taking a closer look! I've noticed you have removed the HTTP comment remark, is that on purpose? |
Yes, because it is not needed and the integration test confirms it. Take notice: #if NETCOREAPP3_1
// Adding the OtlpExporter creates a GrpcChannel.
// This switch must be set before creating a GrpcChannel when calling an insecure HTTP/2 endpoint.
// See: https://docs.microsoft.com/aspnet/core/grpc/troubleshoot#call-insecure-grpc-services-with-net-core-client
if (protocol == OtlpExportProtocol.Grpc)
{
AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true);
}
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
^ There are backends which doesn't support Grpc. |
Add HTTP protocol test case to OTLP exporter integration tests to help with #2691
Remarks
is only needed for the gRPC OTLP protocol (on .NET 3.1 Core of course).