Skip to content
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

[Logging.EventSource] Add trace correlation fields #103655

Merged
merged 6 commits into from
Jun 22, 2024

Conversation

CodeBlanch
Copy link
Contributor

Fixes #76718

Changes

  • Adds ActivityTraceId, ActivitySpanId, and ActivityTraceFlags from Activity.Current to log events

/cc @noahfalk @tarekgh

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 18, 2024
@tarekgh tarekgh self-requested a review June 18, 2024 17:53
@tarekgh tarekgh added this to the 9.0.0 milestone Jun 18, 2024
string? activityTraceId;
string? activitySpanId;
string? activityTraceFlags;
if (activity != null && activity.IdFormat == ActivityIdFormat.W3C)
Copy link
Member

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?

Copy link
Contributor Author

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 out 00000000000000000000000000000000 and SpanId is going to spit out 0000000000000000. We could just push Activity.Id into the events which would include W3C, Hierarchical, and (presumably) anything added in the future. BUT. Accessing Activity.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 the Id 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been supporting the Hierarchical cases with external logger scopes.

public static string GetSpanId(this Activity activity)

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 the Hierarchical cases or not.

Copy link
Contributor Author

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 with ActivityTraceId, ActivitySpanId, and ActivityTraceFlags 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.

Copy link
Member

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.

Copy link
Member

@tarekgh tarekgh left a 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.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@tarekgh tarekgh merged commit 649626b into dotnet:main Jun 22, 2024
74 of 81 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventSourceLogger does not propagate SpanId and TraceId
3 participants