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

Add standard tags to HttpClient native trace instrumentation #104251

Merged
merged 17 commits into from
Jul 10, 2024

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Jul 1, 2024

Add the following standard tags to the HTTP Request Activities started in DelegatingHandler:

http.request.method
http.request.method_original
server.address
server.port
url.full

error.type
http.response.status_code
network.protocol.version

Stable attributes not being added for now, they are not being included by the OTel SDK either:

http.request.resend_count
network.protocol.name
network.peer.port

Just like in #103769, url.full is being redacted by removing UserInfo and the query string, while exposing a System.Net.Http.DisableQueryRedaction switch for opting-out from the latter. This is equivalent to the built-in redaction of OTel SDK, except that the query string is being replaced by a single * instead of replacing values only (key1=*&key2=*), since this is more performant.

Given that the vast majority of current users of HttpClient's distributed tracing relies on the OTel SDK, this redaction technique should not regress those users. In .NET 10 we plan to implement something configurable.

Contributes to #93019, except the enrichment and the propagator aspects which are no longer realistic to address for .NET 9.

@vishweshbankwar note that this is breaking the SDK, since it should now conditionally compile against .NET 9+ so it doesn't double job adding tags. PTAL if it meets your expectations.

cc @samsp-msft @noahfalk

PS: For Aspire dashboard output see #104251 (comment)

@vishweshbankwar
Copy link
Contributor

@antonfirsov - Are you planning to add support for this?

@joegoldman2
Copy link
Contributor

@antonfirsov @vishweshbankwar are there plans to add a feature in the runtime or OTel SDK to enable enrichment of the activity?

https:/open-telemetry/opentelemetry-dotnet-contrib/blob/4c69afa2fbb3b9d84b45bd73dcfad43331516e69/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L147

Using a custom ActivityListener to enrich the activity will not give access to the HttpRequestMessage (or it will be necessary to subscribe to System.Net.Http.HttpRequestOut.Start/Stop events as the OTel SDK does today).

@antonfirsov
Copy link
Member Author

@antonfirsov - Are you planning to add support for this?

@vishweshbankwar does that text mean that the tags should be passed to ActivitySource.(Create|Start)Activity? What should happen if the activity is created via the ctr. /cc @lmolkova

@vishweshbankwar
Copy link
Contributor

vishweshbankwar commented Jul 1, 2024

What should happen if the activity is created via the ctr.

That part would stay same.

@antonfirsov
Copy link
Member Author

are there plans to add a feature in the runtime or OTel SDK to enable enrichment of the activity?

@joegoldman2 enrichment capability will be hopefully added to System.Net.Http in .NET 10. The OTel SDK already exposes callbacks for for that.

@samsp-msft
Copy link
Member

My goals for getting this in (without Enrichment) is so that the instrumentation libraries are not needed for the basic scenarios, especially being able to do auto-instrumentation out of process via EventPipe, such as with .NET Monitor. In that scenario, we want the Activities to have the OTel data directly on them, so when its collected, it has everything that is needed. In that scenario, the application will not have referenced the OTel libraries or setup OTel etc.
The desire to get this into .NET 9 is so that more applications can be monitored using that technique, rather than just those built with the latest runtimes.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. A couple things to consider called out in comments inline. Thanks @antonfirsov!

@cijothomas
Copy link
Contributor

My goals for getting this in (without Enrichment) is so that the instrumentation libraries are not needed for the basic scenarios, especially being able to do auto-instrumentation out of process via EventPipe, such as with .NET Monitor. In that scenario, we want the Activities to have the OTel data directly on them, so when its collected, it has everything that is needed. In that scenario, the application will not have referenced the OTel libraries or setup OTel etc. The desire to get this into .NET 9 is so that more applications can be monitored using that technique, rather than just those built with the latest runtimes.

Thanks for the additional context!
I wonder what should happen to the OTel's instrumentation library for HttpClient? Would it continue to exist for .NET 9 and newer versions, and provide enrichment/filtering capability for users?

@antonfirsov
Copy link
Member Author

Works with Aspire dashboard as expected:

image

@lmolkova
Copy link

lmolkova commented Jul 5, 2024

Works with Aspire dashboard as expected:

🥇

Thanks!

Just noticed: could we also set display name to method? (the one in the http.request.method if it's a known method or HTTP if not known)

The {method} MUST be {http.request.method} if the method represents the original method known to the instrumentation. In other cases (when {http.request.method} is set to _OTHER), {method} MUST be HTTP.

link

@antonfirsov
Copy link
Member Author

could we also set display name to method

@lmolkova does that count as a breaking change?

@lmolkova
Copy link

lmolkova commented Jul 5, 2024

@lmolkova does that count as a breaking change?

The OperationName is still the same, but more importantly - did anyone use plain HTTP client instrumentation before?

Adding @vishweshbankwar in case he has any thoughts.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

LGTM

@vishweshbankwar
Copy link
Contributor

@lmolkova does that count as a breaking change?

The OperationName is still the same, but more importantly - did anyone use plain HTTP client instrumentation before?

Adding @vishweshbankwar in case he has any thoughts.

Agree with @lmolkova. In general, I have only seen OperationName being used in older SDKs which won't be impacted with this.

@vishweshbankwar
Copy link
Contributor

My goals for getting this in (without Enrichment) is so that the instrumentation libraries are not needed for the basic scenarios, especially being able to do auto-instrumentation out of process via EventPipe, such as with .NET Monitor. In that scenario, we want the Activities to have the OTel data directly on them, so when its collected, it has everything that is needed. In that scenario, the application will not have referenced the OTel libraries or setup OTel etc. The desire to get this into .NET 9 is so that more applications can be monitored using that technique, rather than just those built with the latest runtimes.

Thanks for the additional context! I wonder what should happen to the OTel's instrumentation library for HttpClient? Would it continue to exist for .NET 9 and newer versions, and provide enrichment/filtering capability for users?

OTel instrumentation library would continue to exist and provide enrichment/filtering. In terms of implementation, it will no longer need to set tags/status on activity.

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

/ba-g CI failures are unrelated and/or known, eg #104650 or #63224

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 9.0 Native Trace Instrumentation Support for HttpClient as per OTel specification
10 participants