feat: Fail fast when insecure channel is not configured for .NET Core 3.1 with gRPC

[tomkerkhove]

Signed-off-by: Tom Kerkhove kerkhove.tom@gmail.com

Fixes #2690.

Changes

Fail fast when unsecure channel is not configured for .NET Core 3.1 which is required for the OTLP exporter to work correctly.

This was not configured correctly and costed me a few hours to remind myself to configure this.

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

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

[tomkerkhove]

Finalizing running tests locally as well, but feel free to trigger the CI for this as well.

I have not tested the HTTP protocol properly

[codecov]

Codecov Report

Merging #2691 (b7446aa) into main (fd2aa0b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2691   +/-   ##
=======================================
  Coverage   83.81%   83.82%           
=======================================
  Files         250      251    +1     
  Lines        8817     8828   +11     
=======================================
+ Hits         7390     7400   +10     
- Misses       1427     1428    +1     

Impacted Files	Coverage Δ
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs	50.00% <100.00%> (+3.33%)	⬆️
...mentation/ExportClient/ExporterClientValidation.cs	100.00% <100.00%> (ø)
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	79.41% <0.00%> (-2.95%)	⬇️

[tomkerkhove]

This has not been my best PR, sorry @pellared 🤦‍♂️

[tomkerkhove]

@pellared As you can see the build directive is not respected in https://github.com/open-telemetry/opentelemetry-dotnet/runs/4350754547?check_suite_focus=true

As per https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#conditional-compilation you'll notice that NETCOREAPP3_1 is no longer supported as per .NET 5+ SDK which was why I went with the not standard nor .NET 5 or above approach

[pellared]

@pellared As you can see the build directive is not respected in https://github.com/open-telemetry/opentelemetry-dotnet/runs/4350754547?check_suite_focus=true

As per https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#conditional-compilation you'll notice that NETCOREAPP3_1 is no longer supported as per .NET 5+ SDK which was why I went with the not standard nor .NET 5 or above approach

I see but now it fails on .NET Core 3.1 🤦

[tomkerkhove]

Ok so I see the issue here, I think, is that while the Docker Compose file is getting the correct target framework & SDK version (https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/build/docker-compose.netcoreapp3.1.yml#L7-L8), it is using a .NET 6 base image:

Step 1/12 : ARG SDK_VERSION=6.0
Step 2/12 : FROM mcr.microsoft.com/dotnet/sdk:6.0-focal AS build

Details: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Dockerfile#L6

So that means that the directive that I've switched back to per #2691 (comment) is not available and thus not running the new validation. So I'd suggest going back to using this:

#if !NETSTANDARD2_1 && !NET5_0_OR_GREATER instead of #if NETCOREAPP3_1, or change the image for .NET 3.1 tests (less ideal)

[tomkerkhove]

Changed it already but of course the check is now failing for .NET Framework.

Chatted with @pellared offline and he raised a valid concern - Given we use .NET 6 as a base image we might have other .NET Core 3.1 build directives that are being ignored.

What I would propose, @cijothomas, would be to:

1. Use #if NETCOREAPP3_1 in the code
2. Change the integration test workflow on (https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/.github/workflows/integration.yml#L92) so that there is another Dockerfile being used for .NET Core 3.1 so that it's not using .NET 6 but 3.1 instead as this is required per https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#conditional-compilation

That way we explicitly target this to only run for .NET Core 3.1, and be able to test this.

[tomkerkhove]

The test is still failing and I will check tomorrow;

[alanwest]

The test is still failing and I will check tomorrow;

If I were to guess it's the enabling of the switch by this test that causes a conflict with the ConstructingGrpcExporterFailsWhenHttp2UnencryptedSupportIsNotConfiguredForNetcoreapp31 test

https://github.com/open-telemetry/opentelemetry-dotnet/pull/2691/files#diff-ea318d753fea852e284d62944e2e6293f83e44cfb12628b3594685e7f13f1763L36-L44.

It's probably safe to just remove the ConstructingGrpcExporterFailsWhenHttp2UnencryptedSupportIsNotConfiguredForNetcoreapp31 test. I think the two tests that explicitly enable and disable the switch should cover us.

[tomkerkhove]

This must have been my worst PR ever, but hey, it looks like we are ready :)

Thanks for the help!

[pellared]

Is it not worth creating an issue to remove this feature when .NET Core 3.1 is no longer supported?

[tomkerkhove]

PTAL @pellared, @alanwest and @cijothomas

[tomkerkhove]

@pellared are those the final remarks or do you want to do a full review?

I'm happy to keep on changing things but at some point we'll need to just complete the review.

[pellared]

I made a full review. That is also why I approved the PR.

approach changed

[tomkerkhove]

I made a full review. That is also why I approved the PR.

Thanks.

[tomkerkhove]

Any thoughts on this @cijothomas ?