-
Notifications
You must be signed in to change notification settings - Fork 838
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
Adding distinct activity for distributed tracing to YARP #2098
Conversation
…o improve the lifetime.
If you add Re: Tags like routeId, is there prior art in adding such information to existing Activities? Things like that should be trivial to do now that ActivitySource.AddActivityListener(new ActivityListener()
{
ShouldListenTo = source => source.Name == "System.Net.Http",
ActivityStarted = activity =>
{
string routeId = "..."; // Get that somehow (IHttpContextAccessor?)
activity.AddTag("YarpRoute", routeId);
}
}); |
HttpClientInstrumentation is added as part of the AzureMonitor helper. Adding to the existing activity verses adding another is an interesting question. You don't even need to get to the listener, you can probably just do Activity.Current.SetTag(...) |
Other cleanup based on comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, just needs some cleaup.
var activity = Observability.YarpActivitySource.StartActivity("Proxy Forwarder", ActivityKind.Server); | ||
context.SetYarpActivity(activity); | ||
|
||
await _next(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will now allocate an extra async state machine even if the request isn't being sampled.
Can you please change the logic to something along the lines of what I shared here: #2098 (comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will now allocate an extra async state machine even if the request isn't being sampled. Can you please change the logic to something along the lines of what I shared here: #2098 (comment)?
It only adds it if the activity is created, which depends on the listener. If the activity is null, nothing gets set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that the method is now async
, which means you get an extra state machine allocation even if you don't have the activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want you can keep it as-is and I can clean it up in a follow up PR
Adding distinct tracing objects to YARP.
These can then be picked up via monitoring middleware such as Open Telemetry.
The activity source is only active if something is listening to it, such as
t.AddSource("Yarp.*");
.