Add EnableGrpcAspNetCoreSupport option for ASP.NET Core instrumentation #1423
Conversation
| } | ||
| else | ||
| { | ||
| Assert.NotNull(activity.GetTagValue(GrpcTagHelper.GrpcMethodTagName)); |
There was a problem hiding this comment.
as these are added not by Otel Instrumentation, do we need to validate this?
There was a problem hiding this comment.
My original thought was that since the instrumentation removes these tags when this option is enabled it made sense to me to validate that the instrumentation left them untouched when when the option is disabled.
| @@ -1,6 +1,6 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>netstandard2.0</TargetFrameworks> | |||
| <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks> | |||
There was a problem hiding this comment.
@JamesNK We're now targeting netstandard2.1 and conditionally compiling the gRPC functionality for this instrumentation. However, I did note that the Grpc.AspNetCore library targets netcoreapp3.0.
https://github.com/grpc/grpc-dotnet/blob/ef77760676ce19c279d5480bca68af3ea4d60e96/src/Grpc.AspNetCore/Grpc.AspNetCore.csproj#L8
Would it make more sense for us to target netcoreapp3.0 as well?
There was a problem hiding this comment.
netcoreapp3.0 supports netstandard2.1. It doesn't matter either way.
There was a problem hiding this comment.
Ok, thanks! That's what I figured, but just wanted to be sure.
Codecov Report
@@ Coverage Diff @@
## master #1423 +/- ##
==========================================
- Coverage 81.28% 81.24% -0.04%
==========================================
Files 227 227
Lines 6113 6117 +4
==========================================
+ Hits 4969 4970 +1
- Misses 1144 1147 +3
|
| @@ -289,6 +292,13 @@ private static string GetUri(HttpRequest request) | |||
| return builder.ToString(); | |||
| } | |||
|
|
|||
| private static void SetStatusFromHttpStatusCode(Activity activity, int statusCode) | |||
There was a problem hiding this comment.
Might be worth inlining SetStatusFromHttpStatusCode & TryGetGrpcMethod they are only called a couple of places.
There was a problem hiding this comment.
I also applied inline attribute to AddGrpcAttributes. Still kind of trying to feel out when best to apply the inlining attribute. I guess it's technically a balance between code size and performance. Does your intuition usually guide you to apply it to methods with few callers?
There was a problem hiding this comment.
I guess it's technically a balance between code size and performance.
Totally! I think for OpenTelemetry our mission is to have as little overhead on the hosting process as possible so I tend to lean towards performance over binary size. I kind of ask myself: Is this hot path? Is this method really just to keep the code clean? Is it called in only a couple of spots (or is it really tiny)? If the answer is yes to all of that, I usually add the inline hint.
|
So I know the why for why the W3C trace context integration tests are failing but do not yet have the answer for how to best solve it. The test relies on building the |
| FROM mcr.microsoft.com/dotnet/core/sdk:${SDK_VERSION} AS build | ||
| FROM mcr.microsoft.com/dotnet/core/sdk:3.1 AS build |
There was a problem hiding this comment.
@eddynaka Not sure I solved this in the best way. I hardcoded 3.1 SDK for the build stage and then left the final stage using the SDK_VERSION arg. Can you think of a better way to deal with this?
There was a problem hiding this comment.
That's the way. Since 2.1 only exists for adk 3.*, we will have to use that to build. The important part is the version below. So we are still testing for both.
Fixes #1403.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes