Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement ISupportExternalScope in SerilogLoggerProvider #246

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

david-obee
Copy link

@david-obee david-obee commented Mar 13, 2024

Addresses the issue #245.

This adds support in the SerilogLoggerProvider for the ISupportExternalScope interface. The Microsoft.Extensions.Logging library uses this interface when implemented to provide external scopes to logs. In practice, these scopes can include information about current Activity, such as the span ID, trace ID, tags, baggage, etc, which is configured by the ActivityTrackingOptions in the M.E.L library.

Support for tags and baggage via ActivityTrackingOptions was added in dotnet/runtime#48722.

I've left in the sample app SampleActivityLogging which I used to confirm this was possible, which also demonstrates that this change works correctly with the rest of M.E.L and indeed logs out tags and baggage.

If you were to run the SampleActivityLogging program before this change, you'd see the log line

{"Timestamp":"2024-03-13T15:46:52.6901279+00:00","Level":"Information","MessageTemplate":"Hello world!","TraceId":"d80a2e8ab54cfa140fff9cd2ec7458fd","SpanId":"f3f36fa6182fdb3b","Properties":{"SourceContext":"Program","Scope":["{ User = Hugh Mann, Time = 13/03/2024 15:46:52 +00:00 }"]}}

After this change, this is now:

{"Timestamp":"2024-03-13T15:29:20.8566575+00:00","Level":"Information","MessageTemplate":"Hello world!","TraceId":"4190732ed45816ada2d1153755f64415","SpanId":"2af23a9829387a6d","Properties":{"SourceContext":"Program","SpanId":"2af23a9829387a6d","TraceId":"4190732ed45816ada2d1153755f64415","ParentId":"0000000000000000","TraceState":null,"TraceFlags":"Recorded","tag.domain.id":1234,"baggage.environment":"uat","Scope":["{ User = Hugh Mann, Time = 13/03/2024 15:29:20 +00:00 }"]}}

where the I've switched on all the available ActivityTrackingOptions.

Outstanding questions

  • There is already existing logic in Serilog to store the SpanId and TraceId on the LogEvent, and logic in this library in SerilogLogger to supply these values. I imagine we'd want to leave that alone for both backwards compatibility, and for users not using M.E.L? Instead, I think users should just not set those settings in ActivityTrackingOptions to avoid duplication.
  • I've left in my extra sample project for the time being, so anyone can play around with this. I can remove this though should this PR get merged.

@nblumhardt
Copy link
Member

Thanks, David.

This looks good to me. I'm a bit wary of how many allocations we seem to end up doing in that enrichment method - but that issue's separate from what your PR is addressing, so I'll look at it separately.

I think leaving the sample is fine, but perhaps it should be SampleWithExternalScope? Otherwise all LGTM 👍

@david-obee
Copy link
Author

Thanks Nicholas, sounds good. I've updated the test project name.

@nblumhardt nblumhardt merged commit 3b56cb4 into serilog:dev Mar 14, 2024
1 check passed
@nblumhardt
Copy link
Member

🎉

@david-obee david-obee deleted the support-external-scopes branch March 15, 2024 10:32
@nblumhardt nblumhardt mentioned this pull request Dec 6, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants