Adding a http.route Activity tag for HTTP Triggers when OTel is enabled
#10783
Conversation
…he Span name clearer
| if (context.FunctionMetadata?.Trigger?.Raw != null && context.FunctionMetadata.Trigger.Type == "httpTrigger" && context.FunctionMetadata.Trigger.Raw.TryGetValue("route", StringComparison.OrdinalIgnoreCase, out var value)) | ||
| { | ||
| string routeValue = value.ToString(); | ||
| Activity.Current.DisplayName = $"{Activity.Current.DisplayName} {routeValue}"; |
There was a problem hiding this comment.
is Activity.Current guaranteed here?
Also, wonder if we can rely on its value being http method (something sets it to {METHOD}, but what if it starts to set route too? Or, on older .NET versions it'd likely default to activity name, which would be HttpRequestIn or something similar, but not so friendly.
The tl;dr: do we have method name around? can we format the display name that's guaranteed to comply?
There was a problem hiding this comment.
Thanks for taking a look @lmolkova :)
The Host that executes this part of the code for httpTriggers is a .NET 8 Web host, so I'm pretty sure if the activity exists and the trigger is an httpTrigger we can count on it being correct. Also, you can see that the activity originates from the Microsoft.AspNetCore OTel library.
I hope @RohitRanjanMS can correct me if I'm wrong.
I'll add a null check on the Activity.Current, I missed that one :)
There was a problem hiding this comment.
Thanks, @stebet , I will have a look at this today.
There was a problem hiding this comment.
right, so what if in the future version Functions would use routing on the worker? Also, if I'm not mistaken, on .NET 8 OTel instrumentations sets ASP.NET Core activity name, but in .NET 9 (or 10, don't remember), it will be the ASP.NET Core itself. Plus some users may enable ASP.NET Core instrumentation and others won't, so it'd be safer to format the whole thing and and not rely on DisplayName matching method.
There was a problem hiding this comment.
End users don't have control of this host process except through host.json config (and otel currently is just an on/off setting) AFAIK. It'll always be at the root since it handles the reverse proxying and routing of incoming function requests.
Also the OTEL host telemetry is in preview ATM and I'm just looking to make the existing spans it puts out a little more readable.
|
Hi @stebet , let me figure out why http.route is null. If I can fix it, it would be ideal. |
Making OTel span names more clear for HTTP triggers and adding the
http.routetag to the root ActivityThis PR Adds the
http.routeas discussed in #10560 and makes the span name more clear. Currently there aren't any test specifically for the OTel tags, but I have tested this locally with an existing function app.This give us a much clearer root span name like this:
Earlier the root span name would have just been
GET. Now the span has theHttpTrigger.Routeadded to make it more clear which function is being hit as well as the OTel attribute.Tagging @RohitRanjanMS since he seems to have been involved with most of the OTel PRs so far and @lmolkova for visibility as well.
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.md