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

Validate OTLP exporter continues to export after RpcException #3788

Merged
merged 5 commits into from
Oct 25, 2022

Conversation

alanwest
Copy link
Member

Attempt to reproduce #3740.

This PR introduces a mock OpenTelemetry collector that can mimic failed requests. It's run via docker-compose, but could have been done using the in-process ASP.NET Core test framework. However, my hope is that we can expand on this to also test .NET Framework.

The test configures the mock collector to first respond with an Unimplemented status code followed by an OK on the subsequent request.

@alanwest alanwest requested a review from a team October 19, 2022 00:27
@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #3788 (cbc75fb) into main (c5cb95e) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3788   +/-   ##
=======================================
  Coverage   87.41%   87.41%           
=======================================
  Files         280      280           
  Lines       10755    10755           
=======================================
+ Hits         9401     9402    +1     
+ Misses       1354     1353    -1     
Impacted Files Coverage Δ
...Propagators/OpenTelemetryPropagatorsEventSource.cs 87.50% <0.00%> (-12.50%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...lementation/SqlClientInstrumentationEventSource.cs 75.00% <0.00%> (+4.16%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+5.88%) ⬆️

@cijothomas
Copy link
Member

@alanwest do we plant to merge this, and build on more tests based on this foundation? The existing integration tests (which uses a real collector) should be sufficient for most cases, we need to add more, only if it has shortcomings, that can be covered here...

@alanwest
Copy link
Member Author

do we plant to merge this, and build on more tests based on this foundation?

Yes, I think this will provide a good foundation for testing retry #1779. We need a "bad acting" collector - one that can return error status codes. I'd like to implement retry soon.

@cijothomas
Copy link
Member

@cijothomas
Copy link
Member

It's run via docker-compose, but could have been done using the in-process ASP.NET Core test framework. However, my hope is that we can expand on this to also test .NET Framework.

It's run via docker-compose, but could have been done using the in-process ASP.NET Core test framework. However, my hope is that we can expand on this to also test .NET Framework.

The existing test server runs in both .NET FW and .NET. (its a separate issue to check why we did not leverage asp.net core for .NET tests!)

@alanwest
Copy link
Member Author

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/OpenTelemetry.Tests/Shared/TestHttpServer.cs - Can we leverage this?

This is something I looked into in the past when I implemented the original integration test with the real collector. That is, my first choice was to try to avoid using docker. However, as I recall, HttpListener has limitations with regards to HTTP2 and gRPC. I'm trying to dig up my findings from the past, but no luck yet... I'd be happy to dig in again and retrace my steps.

@cijothomas
Copy link
Member

, HttpListener has limitations with regards to HTTP2 and gRPC.

Got it, if this is specifically to cover .NET FW apps. Else we can use Asp.Net Core host, which can cover gRPC/Http2.

@alanwest
Copy link
Member Author

Got it, if this is specifically to cover .NET FW apps. Else we can use Asp.Net Core host, which can cover gRPC/Http2.

Correct. the ideal non-docker solution would use ASP.NET Core host for .NET and something like HttpListner for .NET Framework.

I refreshed my memory a bit on HttpListener. On Windows, HttpListener relies on Http.Sys. Http.Sys does support Http/2 on recent versions of Windows. It requires https which creates a bit of a headache for generating test certs for the CI. But the real blocker is that HttpListener provides no way to set trailing metadata and a gRPC status code.

Perhaps we just punt on concerning ourselves with testing .NET Framework.

@cijothomas
Copy link
Member

Perhaps we just punt on concerning ourselves with testing .NET Framework

This would be an option as well...

@alanwest
Copy link
Member Author

This would be an option as well...

That's fair. I'll rework things to just test .NET using the in-process host.

@cijothomas cijothomas merged commit a3867bd into open-telemetry:main Oct 25, 2022
@alanwest alanwest deleted the alanwest/mock-otlp-test branch October 25, 2022 20:00
@alanwest
Copy link
Member Author

@cijothomas decided to just merge it? Good by me, though I was going to un-dockerize it.

@cijothomas
Copy link
Member

@cijothomas decided to just merge it? Good by me, though I was going to un-dockerize it.

Sorry I was thinking and my mouse clicked merge.. (have a new feature enable to auto-click...sometimes causes things like this)

@alanwest
Copy link
Member Author

Sorry I was thinking and my mouse clicked merge.. (have a new feature enable to auto-click...sometimes causes things like this)

No worries! I'll open a separate PR with the changes.

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.

4 participants