Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

@CodeBlanch
Copy link
Contributor

Fixes#76718

Changes

  • AddsActivityTraceId,ActivitySpanId, andActivityTraceFlags fromActivity.Current to log events

/cc@noahfalk@tarekgh

@dotnet-policy-servicedotnet-policy-servicebot added the community-contributionIndicates that the PR has been added by a community member labelJun 18, 2024
@tarekghtarekgh self-requested a reviewJune 18, 2024 17:53
@tarekghtarekgh added this to the9.0.0 milestoneJun 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
ContributorAuthor

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.

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 theHierarchical cases with external logger scopes.

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.

Copy link
ContributorAuthor

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.

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

@tarekghtarekgh 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

@noahfalknoahfalk 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
Copy link
Member

rzikm pushed a commit to rzikm/dotnet-runtime that referenced this pull requestJun 24, 2024
* Add trace correlation fields to Microsoft.Extensions.Logging.EventSource.* Code review.* Code review.* Test fix.
@CodeBlanchCodeBlanch deleted the logging-eventsource-correlationfields branchJune 25, 2024 20:18
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes

@tarekghtarekghtarekgh approved these changes

Assignees

No one assigned

Labels

area-Extensions-Loggingcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

EventSourceLogger does not propagate SpanId and TraceId

3 participants

@CodeBlanch@tarekgh@noahfalk

[8]ページ先頭

©2009-2025 Movatter.jp