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

Enable .net5.0 #1486

Merged
merged 9 commits into from
Nov 9, 2020
Merged

Enable .net5.0 #1486

merged 9 commits into from
Nov 9, 2020

Conversation

eddynaka
Copy link
Contributor

@eddynaka eddynaka commented Nov 6, 2020

Fixes #.

Changes

Please provide a brief description of the changes here.

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

@eddynaka
Copy link
Contributor Author

eddynaka commented Nov 6, 2020

@alanwest @cijothomas , we might have some issues here....
So, that looks like its happening in gRPC only. The quantity of activities changed (check the test GrpcAndHttpClientInstrumentationIsInvoked).

don't know yet what else is happening...

@eddynaka
Copy link
Contributor Author

eddynaka commented Nov 6, 2020

And the second thing that I saw failing is this: GrpcPropagatesContextWithSuppressInstrumentation. And this was not for this branch...any one knows what might be the issue?

@eddynaka eddynaka self-assigned this Nov 6, 2020
@alanwest
Copy link
Member

alanwest commented Nov 7, 2020

So, that looks like its happening in gRPC only. The quantity of activities changed (check the test GrpcAndHttpClientInstrumentationIsInvoked).

I tracked this down to the fact that HttpClient instrumentation does not get invoked when using Grpc.Net.Client on .NET 5. See #1490 for more details.

Few options for moving forward with this PR in the meantime:

  1. We could hold off on porting the gRPC test suite to .NET 5.
  2. This test could temporarily be fixed to work on .NET 5 by configuring the GrpcChannel as follows:
using var channel = GrpcChannel.ForAddress(uri, new GrpcChannelOptions()
{
    HttpClient = new HttpClient(),
});

@alanwest
Copy link
Member

alanwest commented Nov 7, 2020

And the second thing that I saw failing is this: GrpcPropagatesContextWithSuppressInstrumentation. And this was not for this branch...any one knows what might be the issue?

Ugh, I noted this issue as well on your branch when running all the gRPC tests concurrently. Seems concurrency issues keep cropping up with this test suite. I have not yet tracked down the cause, but I will keep poking at it...

@eddynaka
Copy link
Contributor Author

eddynaka commented Nov 7, 2020

So, that looks like its happening in gRPC only. The quantity of activities changed (check the test GrpcAndHttpClientInstrumentationIsInvoked).

I tracked this down to the fact that HttpClient instrumentation does not get invoked when using Grpc.Net.Client on .NET 5. See #1490 for more details.

Few options for moving forward with this PR in the meantime:

  1. We could hold off on porting the gRPC test suite to .NET 5.
  2. This test could temporarily be fixed to work on .NET 5 by configuring the GrpcChannel as follows:
using var channel = GrpcChannel.ForAddress(uri, new GrpcChannelOptions()
{
    HttpClient = new HttpClient(),
});

Should we add this workaround for now? What do you think?

@alanwest
Copy link
Member

alanwest commented Nov 8, 2020

Should we add this workaround for now? What do you think?

I'm ok with this workaround for now. Maybe inside of a #if NET5_0 block with a comment linking to #1490 just so it's clear why.

@eddynaka
Copy link
Contributor Author

eddynaka commented Nov 8, 2020

Should we add this workaround for now? What do you think?

I'm ok with this workaround for now. Maybe inside of a #if NET5_0 block with a comment linking to #1490 just so it's clear why.

just added a if for netcoreapp3.1, since net5_0 wasn't working for me.

@cijothomas cijothomas marked this pull request as ready for review November 9, 2020 20:20
@cijothomas cijothomas requested a review from a team November 9, 2020 20:20
@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #1486 (7c24a5c) into master (bebc80f) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1486      +/-   ##
==========================================
+ Coverage   81.13%   81.15%   +0.01%     
==========================================
  Files         232      232              
  Lines        6309     6309              
==========================================
+ Hits         5119     5120       +1     
+ Misses       1190     1189       -1     
Impacted Files Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs 84.80% <0.00%> (-0.80%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 100.00% <0.00%> (+2.85%) ⬆️
...us/Implementation/PrometheusExporterEventSource.cs 72.72% <0.00%> (+9.09%) ⬆️

@cijothomas cijothomas merged commit 052ecc4 into open-telemetry:master Nov 9, 2020
@eddynaka eddynaka deleted the feature/enabling-net5 branch November 9, 2020 20:35
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