Skip to content

Conversation

@jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Mar 28, 2022

Fixes #

Context

In #7390 I modified some of the event source methods but I just noticed that things look wrong in the latest internal preview. It seems like Start/Stop event methods need to have the same version in order to be linked up.

image
image

I'm working on event messages like this in NuGet and just used a constant to rev the version of all event methods at the same time which seems like a better practice in general.

Changes Made

Add a constant to represent the event method version and use that version in all event methods.

Testing

Manually tested locally, these are the events logged now:

image

Here is a sample for others to verify as well:

PerfViewData.etl.zip

Notes

@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 28, 2022

I think I'll never touch the event source in MSBuild ever again, somehow I keep messing it up...

Copy link
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Does this first version update update any method signature changes that already exist? If so, I'd say it makes sense to just update the version of a particular event whenever we need to. This way leads to extra events being updated unnecessarily.

Edit: Sounds like yes, so I'd slightly prefer assigning them all (individually) version 1 (or 100) and updating start/stop pairs together but separately from the rest.

/// <summary>
/// Represents the version of the events. Be sure to increment this any time you modify an existing event method.
/// </summary>
public const int EventVersion = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some reason you starting at 100 instead of 1? Does the version comparer think 9 > 10?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping 😄

@jeffkl
Copy link
Contributor Author

jeffkl commented Apr 18, 2022

I spent a few hours trying to understand ETW more and when and when not to use this Version attribute metadata and couldn't find anything obvious. I don't have any more time to spend on this so I'm going to close this for now since my solution doesn't seem desirable.

@jeffkl jeffkl closed this Apr 18, 2022
@jeffkl jeffkl deleted the fix-event-source-version branch April 18, 2022 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants