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

[EventTrace] AddEventPipe filter data parsing #109624

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Nov 7, 2024

Fixes #102572

This PR follows-up on #102612 (comment) and #102612 (comment) by extending the FilterData parsing to cover EventPipe scenarios.

Testing

Checked the value of l64ClientSequenceNumber after enabling an EventPipe session with dotnet trace collect -n sampleapp --providers "Microsoft-Windows-DotNETRuntime:800000:5:mihw=whim;GCSeqNumber=2024" and saw 0x7e8.

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.

Some suggestions inline, but the approach seems good.

src/native/eventpipe/ep-provider.c Outdated Show resolved Hide resolved
src/coreclr/vm/eventtrace.cpp Show resolved Hide resolved
src/coreclr/vm/eventtrace.cpp Show resolved Hide resolved
src/coreclr/nativeaot/Runtime/eventtrace.cpp Show resolved Hide resolved
Only keep implemented filter types in enum
Add EventPipe parsing to nativeaot counterpart
Add bounds check on value portion of buffer
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.

I think you may have an off-by-one error, but looks good otherwise 👍

@mdh1418 mdh1418 merged commit bb5baa8 into dotnet:main Nov 12, 2024
131 checks passed
@mdh1418 mdh1418 deleted the fix_trace_event_filter_data_parsing branch November 12, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants