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

[Breaking change] Changing EventSource OnEventCommand to be consistent #84586

Closed
davmason opened this issue Apr 10, 2023 · 5 comments
Closed
Assignees
Labels
area-Tracing-coreclr breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Comments

@davmason
Copy link
Member

PR #82970 was a fix for issue #61353, it included a minor breaking change in that we make the callback for EventSource.OnEventCommand after we mark all events as disabled.

Previous behavior
Prior to this change, we would issue the EventSource.OnEventCommand callback for a EventCommand.Disable prior to setting m_eventSourceEnabled=false. This means that IsEnabled would return true in the OnEventCommand callback for a user EventSource, even if the command would lead to the EventSource being disabled.

The callback happens after we turn off the ability to dispatch events though, so even if an EventSource tries to fire an event it will not be written. Also, for EventCommand.Enable we issue a consistent view - IsEnabled accurately reports the state of the world, and the EventSource can write events from the OnEventCommand callback. This change makes the EventCommand.Disable behavior match the Enable behavior.

New behavior
The change makes the callbacks consistent, now we will mark the EventSource as disabled before the callback.

@PriyaPurkayastha - We have multiple first party requests to backport this fix to 6.0 and 7.0, I would appreciate guidance on how to proceed with the backports. Is documentation sufficient?

@davmason davmason added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. area-Tracing-coreclr labels Apr 10, 2023
@davmason davmason self-assigned this Apr 10, 2023
@ghost ghost added untriaged New issue has not been triaged by the area owner needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Apr 10, 2023
@ghost
Copy link

ghost commented Apr 10, 2023

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

  1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

@davmason
Copy link
Member Author

davmason commented Apr 10, 2023

CC @dotnet/dotnet-diag @jeffschwMSFT

@davmason
Copy link
Member Author

Actually CC @dotnet/dotnet-diag (typo the first time)

@tommcdon tommcdon removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 17, 2023
@davmason
Copy link
Member Author

Closing this since we have the breaking change doc bugs created

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tracing-coreclr breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

No branches or pull requests

2 participants