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/6.0] Port EventCounters multi session support to 6.0 #84680

Merged
merged 3 commits into from
May 11, 2023

Conversation

davmason
Copy link
Member

Ports #82970 to 6.0

Customer Impact

Currently multiple different EventCounters sessions can be started but when any session stops we stop emitting counters for all events. This is true for dotnet-counters and manually enabling counters via in an process EventListener or via ETW/EventPipe/LTTNG.

We have quite a few internal and external teams using EventCounters, so as time goes on we find that they are disabling each other's sessions more and more. We have received requests from internal and external partners that this be fixed in servicing.

Testing

Partner team validation that it fixes their scenario.

Risk

This fix includes a minor breaking change as described in #84586. We previously would issue callbacks for EventSource Disable events before we fully marked the EventSource as disabled, but after we disallowed any further events from being sent. With this change we will issue callbacks after we mark the EventSource as fully disabled.

I cannot think of a scenario this would break, but it is different behavior and could theoretically cause issues for customers.

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. we will take for consideration in 6.0.x. please get a code review

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Apr 12, 2023
@leecow leecow modified the milestones: 6.0.x, 6.0.18 Apr 13, 2023
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 13, 2023
@jeffschwMSFT jeffschwMSFT added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Apr 13, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 13, 2023
@ghost
Copy link

ghost commented Apr 13, 2023

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

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

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

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.

The change looks correct as-is, though I think more than half the non-assert, non-comment portion of the change is refactoring that doesn't change the final behavior. If you've already spent a bunch of time testing and this would reset your work I wouldn't worry about it, but ideally we'd skip the refactoring and just do the minimal functional change.

@carlossanlop
Copy link
Member

As indicated in the 7.0 PR - Code complete is May 15th for the June Release.

@davmason davmason force-pushed the eventcounters_6.0 branch from 98358d5 to fb6a4af Compare May 9, 2023 20:15
@davmason
Copy link
Member Author

davmason commented May 9, 2023

Rebasing against release/6.0-staging latest to trigger a new CI run

@davmason davmason merged commit 2ab41dc into dotnet:release/6.0-staging May 11, 2023
@cmoaciopm
Copy link

Could some please tell when will 6.0.18 be released? I saw 6.0.18 milestone was already closed. Does that mean it will be released soon? BTW, I found 6.0.17 is not released yet. Not sure whether 6.0.17 will be skipped or not. I'm asking this question because there is breaking change for the fix. Hope someone can help to clarify my question. Thanks.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 21, 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. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants