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

[release/7.0] Managed EventSources do not show up by default in EventPipe sessions (#75248) #77811

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

davmason
Copy link
Member

@davmason davmason commented Nov 2, 2022

Backport of #75248 to release/7.0

Customer Impact

This is customer reported and requires deep understanding to diagnose and understand the 'fix'.

Currently if you specify Keywords==0 on an EventPipe session, you will get no events from managed EventSources by default. This is due to the fact that managed EventSources use bits 44-47 of the keywords field to keep track of which session the event belongs to. An event from a managed EventSource will have keywords 0xF00000000000 by default, even if no keywords are set on the event.

The expected behavior is that if you have a session with no keywords specified you will see any events with no keywords specified.

Testing

Manual verification of dotnet-trace and Microsoft.Diagnostics.NETCore.Client will receive keywords as expected

Risk

We are changing the default behavior of what events are emitted, but we are changing it to match the documentation so it should be a positive change. We have had multiple people run in to this issue and not be able to figure it out on their own.

@davmason davmason added this to the 7.0.0 milestone Nov 2, 2022
@davmason davmason requested review from lateralusX and a team November 2, 2022 23:26
@davmason davmason self-assigned this Nov 2, 2022
Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Do we have any ES in the runtime that have no keywords and that would suddenly get noisy for customers and changes the perf of the scenarios they already measure? I see a lot of them in dotnet.dll, libs like Microsoft.AspNetCore.DeveloperCertificates.XPlat, Microsoft.AspNet.Hosting, kestrel, System.Net.Sockets, ArrayPool... In general this feels like a better approach for how to handle this (unless someone tries to reason about the source of an event with this).

@carlossanlop
Copy link
Member

@davmason can you please send the email to Tactics requesting approval, and add the label servicing-consider?

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. please take a look at the failing ci. we will take for consideration in 7.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Nov 3, 2022
@jeffschwMSFT jeffschwMSFT modified the milestones: 7.0.0, 7.0.x Nov 3, 2022
@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 3, 2022
@jeffschwMSFT jeffschwMSFT modified the milestones: 7.0.x, 7.0.1 Nov 3, 2022
@davmason
Copy link
Member Author

davmason commented Nov 3, 2022

Do we have any ES in the runtime that have no keywords and that would suddenly get noisy for customers and changes the perf of the scenarios they already measure? I see a lot of them in dotnet.dll, libs like Microsoft.AspNetCore.DeveloperCertificates.XPlat, Microsoft.AspNet.Hosting, kestrel, System.Net.Sockets, ArrayPool... In general this feels like a better approach for how to handle this (unless someone tries to reason about the source of an event with this).

I don't think there are any super chatty providers with no keywords, usually if they are super chatty we put them behind keywords. This change makes the behavior match other collection mechanisms, so it shouldn't surprise anyone

@davmason davmason requested a review from hoyosjs November 3, 2022 20:12
@davmason
Copy link
Member Author

davmason commented Nov 3, 2022

For transparency, I just pushed new changes to check for events with all the keyword bits set. This reverts it to the original change I had made in 8.0, I thought I could simplify it but EventSource will send a message with all the keywords set when it runs in to an internal error.

src/native/eventpipe/ep-provider.c Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Nov 3, 2022
2 tasks
Co-authored-by: Juan Hoyos <juan.hoyos@microsoft.com>
@carlossanlop
Copy link
Member

Approved by Tactics.
Signed off by area owners.
No OOB package authoring changes needed.
CI is green.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 2a65e07 into dotnet:release/7.0 Nov 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants