Feature request: Add Activity.Current.Id to ErrorMessage (and maybe Events)

[JustusGreiberORGADATA]

Which version of Duende IdentityServer are you using?
6.3.5

Which version of .NET are you using?
net6.0

Describe the bug feature request

Instrumentation for ActivitySources was added in Version 6.0 of IdentityServer with DuendeArchive/Support#698. When IdentityServer creates an ErrorMessage and redirects to our Error-Controller the current ActivityID is sadly not persisted, which means it is hard to find the correct trace/logs to diagnose the issue. Only the request id is pulled from the HttpContext and saved [0]. It would be really nice if Activity.Current?.Id was saved inside the ErrorMessage, so we can diagnose issues more easily.

Events have the same behavior [1]. But they save the request id in a property named ActivityId, which makes them lot more confusing. In the case of events I could understand if backwards compatibility makes it hard to change this though.

The best solution for me would be to make both of these classes behave like the following:

public partial abstract class Event {
    public string? RequestId { get; set; } = HttpContext.TraceIdentifier;
    public string? ActivtityId { get; set; } = Activity.Current?.Id;
}

public partial abstract class ErrorMessage{
    public string? RequestId { get; set; } = HttpContext.TraceIdentifier;
    public string? ActivtityId { get; set; } = Activity.Current?.Id;
}

[0] https://github.com/DuendeSoftware/IdentityServer/blob/ef64ef16e84881b1eba4b2cf2ffef39e20eb640b/src/IdentityServer/Endpoints/Results/AuthorizeResult.cs#L225
[1] https://github.com/DuendeSoftware/IdentityServer/blob/ef64ef16e84881b1eba4b2cf2ffef39e20eb640b/src/IdentityServer/Services/Default/DefaultEventService.cs#L114

[AndersAbel]

It makes perfect sense to include Activity.Current?.Id in ErrorMessage.

The Events are being phased out in favour of the new open telemetry compatible activities/logs/metrics.

[AndersAbel]

@JustusGreiberORGADATA I've now implemented this and submitted a PR. I first tried using your suggestion:

public string? ActivtityId { get; set; } = Activity.Current?.Id;

However, this had one problem if there is no Current activity in the ErrorMessage that is serialized. Then the deserializer will not write the property at all (because it's null in the payload). That will leave the default value in the object created to receive the values of the deserializer. The end result was that if there was no current activity when the error message was created, the activity Id of when the error was displayed/used would be used on the ErrorMessage. I would expect that if a null activity was passed in to serialization, then a null activity would be read back too.

The more straight forward solution is to let the creator of the ErrorMessage initialize the ActivityId. In IdentityServer that is only done in one place in the AuthorizeHttpWriter. But if custom code creates an ErrorMessage it now needs to be updated to include setting the ActivityId.

[JustusGreiberORGADATA]

@AndersAbel Yeah I agree, the ID should be null, if Activity.Current was null at the time of creation. It should not magically appear later after deserialization.

But if custom code creates an ErrorMessage it now needs to be updated to include setting the ActivityId.

I don't have any code like that. I doubt it is a common use case.