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

[GrpcNetClient] Clarify impact to SuppressDownstreamInstrumentation #5340

Merged

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Feb 9, 2024

We should probably decide what we're actually going to do with SuppressDownstreamInstrumentation, but for the purpose of getting the next release out, I just wanted to clean up our advisory.

@alanwest alanwest requested a review from a team February 9, 2024 22:50
@reyang
Copy link
Member

reyang commented Feb 9, 2024

We should probably decide what we're actually going to do with SuppressDownstreamInstrumentation, but for the purpose of getting the next release out, I just wanted to clean up our advisory.

Would this PR description help on the direction? #960 (comment)

@alanwest
Copy link
Member Author

alanwest commented Feb 9, 2024

Would this PR description help on the direction?

Yes, this is an interesting thought to consider. I believe what you're saying is there's no reason instrumentation needs an SDK dependency to check otel.suppress_instrumentation using RuntimeContext from OpenTelemetry.Api directly. We'd add the check back in the HTTP instrumentation as well.

Does anyone have strong feelings that we pursue this fix prior to releasing 1.7.1 today (#5322)? (@utpilla, @CodeBlanch, @vishweshbankwar)

My opinion is, since this breaking change was introduced last November that we merge this PR with a clear advisory, release 1.7.1 today, and then follow up with this idea.

@reyang
Copy link
Member

reyang commented Feb 9, 2024

Would this PR description help on the direction?

Yes, this is an interesting thought to consider. I believe what you're saying is there's no reason instrumentation needs an SDK dependency to check otel.suppress_instrumentation using RuntimeContext from OpenTelemetry.Api directly. We'd add the check back in the HTTP instrumentation as well.

Correct, it's just a convention/suggestion (instead of a promise) that anyone can choose to follow (e.g. if there is another telemetry SDK, it can choose to respect this flag as well).

@utpilla
Copy link
Contributor

utpilla commented Feb 10, 2024

Does anyone have strong feelings that we pursue this fix prior to releasing 1.7.1 today (#5322)?

No.

My opinion is, since this breaking change was introduced last November that we merge this PR with a clear advisory, release 1.7.1 today, and then follow up with this idea.

I'm fine with that.

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Feb 10, 2024

Would this PR description help on the direction?

Yes, this is an interesting thought to consider. I believe what you're saying is there's no reason instrumentation needs an SDK dependency to check otel.suppress_instrumentation using RuntimeContext from OpenTelemetry.Api directly. We'd add the check back in the HTTP instrumentation as well.

Correct, it's just a convention/suggestion (instead of a promise) that anyone can choose to follow (e.g. if there is another telemetry SDK, it can choose to respect this flag as well).

Would this PR description help on the direction?

Yes, this is an interesting thought to consider. I believe what you're saying is there's no reason instrumentation needs an SDK dependency to check otel.suppress_instrumentation using RuntimeContext from OpenTelemetry.Api directly. We'd add the check back in the HTTP instrumentation as well.

Correct, it's just a convention/suggestion (instead of a promise) that anyone can choose to follow (e.g. if there is another telemetry SDK, it can choose to respect this flag as well).

I believe this(#960 (comment)) is a good option to consider for offering SuppressDownstreamInstrumentation as an experimental feature. Experimental because we also need to think about the case when instrumentation will be done natively by HttpClient (starting with .NET 9.0) and eventually by Grpc.Net.Client as well.

I am good with going ahead with the release for now.

@alanwest alanwest merged commit 8b88f43 into open-telemetry:main Feb 10, 2024
17 checks passed
@alanwest alanwest deleted the alanwest/suppress-clarification branch February 10, 2024 01:25
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