Open RehanSaeed opened 3 years ago
This is something which requires some discussion/design. Current instrumentations populate only the required fields, and any other fields which are readily available (without much perf cost). We need to find a balance between adding all the fields vs adding just required fields, possibly by offering Options to control. One open PR in this direction: https://github.com/open-telemetry/opentelemetry-dotnet/pull/1546
options.Enrich = (activity, eventName, obj) => { if (obj is HttpRequest request) { var user = request.HttpContext.User; if (user.Identity?.Name is not null) { activity.AddTag(OpenTelemetryAttributeName.EndUser.Id, user.Identity.Name); activity.AddTag(OpenTelemetryAttributeName.EndUser.Scope, string.Join(',', user.Claims.Select(x => x.Value))); } } };
Is it even possible to obtain properties of the user this way? I'm just trying to do the same thing and the identity comes always empty, as if at the point where the Enrich
callback is called it wasn't populated yet.
Is it even possible to obtain properties of the user this way? I'm just trying to do the same thing and the identity comes always empty, as if at the point where the Enrich callback is called it wasn't populated yet.
I'm finding the same thing -- just commented over here about it: https://github.com/RehanSaeed/rehansaeed.github.io/issues/607#issuecomment-1129404665
TL;DR The "OnStartActivity" callback is way too early to successfully resolve an authenticated User principal. Authentication middleware hasn't executed yet.
Would love to have some guidance from the experts here about how to enrich telemetry data with "enduser" values. This seems like a common use-case -- something I'd expect to be a solved problem in the AspNetCore Instrumentation library.
The specification recommends propagating "enduser" values via the Baggage mechanism.
To accomplish that today, it seems I'd need to write some custom middleware that runs immediately after Authentication -- to copy the authenticated User ("enduser") data into Baggage -- and then add processors like this sample one to enrich telemetry data from Baggage. Is that the recommendation here?
This is something which requires some discussion/design. Current instrumentations populate only the required fields, and any other fields which are readily available (without much perf cost). We need to find a balance between adding all the fields vs adding just required fields, possibly by offering Options to control. One open PR in this direction: open-telemetry/opentelemetry-dotnet#1546
Ideally all the "optional" but relevant conventions are available but maybe flagged by default? That way there isn't a bunch of people writing redundant code, but they can still control the perf aspects.
Is it even possible to obtain properties of the user this way? I'm just trying to do the same thing and the identity comes always empty, as if at the point where the Enrich callback is called it wasn't populated yet.
I'm finding the same thing -- just commented over here about it: RehanSaeed/rehansaeed.github.io#607 (comment)
TL;DR The "OnStartActivity" callback is way too early to successfully resolve an authenticated User principal. Authentication middleware hasn't executed yet.
Would love to have some guidance from the experts here about how to enrich telemetry data with "enduser" values. This seems like a common use-case -- something I'd expect to be a solved problem in the AspNetCore Instrumentation library.
The specification recommends propagating "enduser" values via the Baggage mechanism.
To accomplish that today, it seems I'd need to write some custom middleware that runs immediately after Authentication -- to copy the authenticated User ("enduser") data into Baggage -- and then add processors like this sample one to enrich telemetry data from Baggage. Is that the recommendation here?
A rather janky solution for this might be an EnrichOpenTelemetryIdentity
that runs after UseAuthentication
and looks for the IHttpAuthenticationFeature
feature in context and hydrates from that. I don't see a way to automatically plug that info however.
If you use jwt in the authorization, you can directly read the claims which usually has the information you need, example: //get token and I remove the start word 'bearer ' sJwt = context.Request.Headers["Authorization"][0].Substring(7) //read token, user_name is the claim in my case if (sJwt != "" && jwtSecurityTokenHandler.CanReadToken(sJwt)) { var jwtSecurityToken = new System.IdentityModel.Tokens.Jwt.JwtSecurityToken(sJwt); var userName = jwtSecurityToken.Payload.Claims.FirstOrDefault(c => c.Type == "user_name")?.Value; if (userName != null) activity.AddTag("enduser.user_name", userName); }
Any updates?!
Any updates?!
Asp.Net Core is considering adding native instrumentation, with capability to enrich/filter etc. There probably isn't much to do in this repo as instrumentation is getting native from asp.net core itself. (Though we may need to do something for older versions of asp.net core)
Since we have the stable instrumentation for AspNetCore with the required attributes, I'm not sure whether there is anything that we should be doing with this ticket?
@cijothomas would you say this is safe to close in that case?
Since we have the stable instrumentation for AspNetCore with the required attributes, I'm not sure whether there is anything that we should be doing with this ticket?
@cijothomas would you say this is safe to close in that case?
Not really..
Instrumentation is probably only doing all the required attributes. What about the optional/opt-in ones? Most likely native instrumentation would offer some capability, but for pre .NET 9 versions, this repo is the only place to offer some ways to get the non-required attributes.
Feature Request
Is your feature request related to a problem?
There are several tags specified by the open telemetry specification that are not logged by default in
AddAspNetCoreInstrumentation
. It would be nice if we could add an option to log them too.Describe the solution you'd like:
I currently use the following code to log them:
Describe alternatives you've considered.
Currently using the
Enrich
method to add additional tags but would be nice if these were built in as they are defined by the specification.