- Notifications
You must be signed in to change notification settings - Fork5.2k
[Logging.EventSource] Add trace correlation fields#103655
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Logging.EventSource] Add trace correlation fields#103655
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/libraries/Microsoft.Extensions.Logging.EventSource/src/EventSourceLogger.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| string?activityTraceId; | ||
| string?activitySpanId; | ||
| string?activityTraceFlags; | ||
| if(activity!=null&&activity.IdFormat==ActivityIdFormat.W3C) |
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.
Are we sure we don't want support non W3C cases too?
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.
Well for non-W3C TraceId is going to spit out00000000000000000000000000000000 and SpanId is going to spit out0000000000000000. We could just pushActivity.Id into the events which would includeW3C,Hierarchical, and (presumably) anything added in the future. BUT. AccessingActivity.Id will force an allocation in the process being monitored. And it will force consumers to have to inspect the value and understand the possible formats/differences. Probably also the consumers will have to chop up theId into the individual components. In my world of OTel I don't need hierarchical but if you feel there is a need and the tradeoffs are OK, I'm happy to switch it.
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.
We have been supporting theHierarchical cases with external logger scopes.
runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactoryScopeProvider.cs
Line 291 inb93061a
| publicstaticstringGetSpanId(thisActivityactivity) |
Also, will it make sense to add the parent Id to the event as we do in scopes?
CC@noahfalk if he has more insight if we should ignore theHierarchical cases or not.
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.
Just FYI the reason I went withActivityTraceId,ActivitySpanId, andActivityTraceFlags is because those are the only things defined on the OTel Logs Data Model for correlation:https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#log-and-event-record-definition
Happy to add other things, but from OTel perspective they aren't required.
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.
I'd be fine omitting support for hierarichal. Worst case I forsee is we get feedback that someone wants Hierarichal support, we decide the scenario in the request is important and then we add that too in a future release. I think that has a pretty low chance of happening.
src/libraries/Microsoft.Extensions.Logging.EventSource/tests/EventSourceLoggerTest.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
tarekgh left a 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.
Let's wait@noahfalk approval and then we can proceed merging.
noahfalk left a 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.
LGTM 👍
tarekgh commentedJun 21, 2024
@CodeBlanch I am seeing test failure in https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-103655-merge-e2f8c48b4c15440fa9/Microsoft.Extensions.Logging.EventSource.Tests/1/console.3e8ced58.log?helixlogtype=result which can be related. |
* Add trace correlation fields to Microsoft.Extensions.Logging.EventSource.* Code review.* Code review.* Test fix.
Fixes#76718
Changes
ActivityTraceId,ActivitySpanId, andActivityTraceFlagsfromActivity.Currentto log events/cc@noahfalk@tarekgh