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.

Sign up for GitHub

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

Jump to bottom

[ASP.NET Core] Remove grpc server instrumentation #5097

Merged
merged 7 commits into from
Nov 29, 2023
Merged

[ASP.NET Core] Remove grpc server instrumentation #5097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a19ac1
Remove grpc server instrumentation
vishweshbankwar Nov 29, 2023
d9c89a0
Merge branch 'main' into vibankwa/remove-grpc-server-instrumentaion
vishweshbankwar Nov 29, 2023
8ecf5b4
rmv using
vishweshbankwar Nov 29, 2023
7e1fe91
Merge branch 'main' into vibankwa/remove-grpc-server-instrumentaion
vishweshbankwar Nov 29, 2023
e8ce821
format
vishweshbankwar Nov 29, 2023
a1bc4ef
changelog
vishweshbankwar Nov 29, 2023
3111a2c
changelog update
vishweshbankwar Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/OpenTelemetry.Instrumentation.AspNetCore/.publicApi/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.AspNetCoreInstrumentationOptions() -> void
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.EnableGrpcAspNetCoreSupport.get -> bool
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.EnableGrpcAspNetCoreSupport.set -> void
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.EnrichWithException.get -> System.Action<System.Diagnostics.Activity, System.Exception>
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.EnrichWithException.set -> void
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.EnrichWithHttpRequest.get -> System.Action<System.Diagnostics.Activity, Microsoft.AspNetCore.Http.HttpRequest>
Expand Down
22 changes: 13 additions & 9 deletions src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreInstrumentationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,17 @@ public class AspNetCoreInstrumentationOptions
/// </remarks>
public bool RecordException { get; set; }

#if NET6_0_OR_GREATER
/// <summary>
/// Gets or sets a value indicating whether RPC attributes are added to an Activity when using Grpc.AspNetCore. Default is true.
/// </summary>
/// <remarks>
/// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md.
/// </remarks>
public bool EnableGrpcAspNetCoreSupport { get; set; } = true;
#endif
/*
* Removing for stable release of http instrumentation.
* grpc semantic conventions are not yet stable so this option will not be part of stable package.
#if NET6_0_OR_GREATER
/// <summary>
/// Gets or sets a value indicating whether RPC attributes are added to an Activity when using Grpc.AspNetCore. Default is true.
/// </summary>
/// <remarks>
/// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md.
/// </remarks>
public bool EnableGrpcAspNetCoreSupport { get; set; } = true;
#endif
*/
}
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
* Removed `netstandard2.1` target.
([#5094](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5094))

* Removed support for grpc instrumentation to unblock stable release of http
instrumentation. For details, see issue
Comment on lines +14 to +15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the decision to comment the functionality out when we could use the EXPOSE_EXPERIMENTAL_FEATURES directive. Seems this causes more toil for us to maintain and for users who may want to use the latest instrumentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to have all the breaking changes introduced in rc version itself so that users are aware of what is coming in stable release. After the stable release, we can re-introduce this as experimental.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that but I think it might be okay to comment this functionality out for the next rc which is supposed to mimic the stable behavior. We could see if people complain about this in the rc and decide to mark it experimental then? Otherwise, we would introduce in the first alpha after the stable release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I'm hearing is that we want to treat rc releases a bit differently than other pre-release monikers like alpha, beta, etc. I'd be fine if we set a precedent that EXPOSE_EXPERIMENTAL_FEATURES is only set for non-rc prereleases. I'm mainly looking for ways we can avoid the commenting/uncommenting toil we've had in the past.

I don't feel strongly that this is performed right now, so if you'd prefer to take the shortcut and comment it out for now then I'm ok with that. Though, I do want to see this reintroduced as soon as possible. I don't need to wait for customers to complain. I know of customers that use this instrumentation. Also, the cartservice from the opentelemetry-demo project is a gRPC service and depends on this instrumentation.

Copy link
Contributor

@utpilla utpilla Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I'm hearing is that we want to treat rc releases a bit differently than other pre-release monikers like alpha, beta, etc.

We want to treat this particular rc that we would be releasing next differently. As this is the first time this library would have a stable release so that's why this rc is special than let's say an rc that would happen after we have had a stable release.

Though, I do want to see this reintroduced as soon as possible. I don't need to wait for customers to complain. I know of customers that use this instrumentation.

We would have to tell them that these conventions are not stabilized yet. Ideally (meaning taking the shortcut route), we would add it right after the first stable which should happen in ~2-3 weeks from now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would add it right after the first stable which should happen in ~2-3 weeks from now

ok

[#5098](https://github.com/open-telemetry/opentelemetry-dotnet/issues/5098)
([#5097](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5097))

## 1.6.0-beta.3

Released 2023-Nov-17
Expand Down
20 changes: 8 additions & 12 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,22 +252,18 @@ public void OnStopActivity(Activity activity, object payload)
#endif

activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode));

#if !NETSTANDARD2_0
if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod))
{
this.AddGrpcAttributes(activity, grpcMethod, context);
}
else if (activity.Status == ActivityStatusCode.Unset)
{
activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, response.StatusCode));
}
#else
/*
#if !NETSTANDARD2_0

if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod))
{
this.AddGrpcAttributes(activity, grpcMethod, context);
}
*/
if (activity.Status == ActivityStatusCode.Unset)
{
activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, response.StatusCode));
}
#endif

try
{
Expand Down
14 changes: 2 additions & 12 deletions test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,9 @@

#if NET6_0_OR_GREATER
using System.Diagnostics;
using System.Net;
using Greet;
using Grpc.Core;
using Grpc.Net.Client;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Moq;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Instrumentation.Grpc.Services.Tests;
using OpenTelemetry.Instrumentation.GrpcNetClient;
using OpenTelemetry.Trace;
using Xunit;
using static OpenTelemetry.Internal.HttpSemanticConventionHelper;
using Status = OpenTelemetry.Trace.Status;

namespace OpenTelemetry.Instrumentation.Grpc.Tests;

Expand All @@ -46,6 +35,7 @@ public GrpcTests()
this.server = new GrpcServer<GreeterService>();
}

/*
[Theory]
[InlineData(null)]
[InlineData(true)]
Expand Down Expand Up @@ -207,7 +197,7 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewAc
}));
}
}

*/
public void Dispose()
{
this.server.Dispose();
Expand Down