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

Add MVID and MethodToken To Parameters #7974

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kkeirstead
Copy link
Member

Summary

Added MVID and MethodToken to Parameter Capture output.

Release Notes Entry

Added ModuleVersionId and MethodToken Fields to Parameter Capture Output

@kkeirstead kkeirstead requested a review from a team as a code owner February 28, 2025 19:08
@schmittjoseph schmittjoseph added experimental-feature Pull requests that are focused on an experimental feature update-release-notes Pull requests that should be mentioned in the release notes labels Feb 28, 2025
@schmittjoseph
Copy link
Member

Let's also add/update a function test for this --src\Tests\Microsoft.Diagnostics.Monitoring.Tool.FunctionalTests\ParameterCapturingTests.cs already has several, and the Exceptions functional tests should already have an example of how to get the expected mvid & mdToken IIRC.

public const int MethodName = 5;
public const int MethodModuleName = 6;
public const int MethodDeclaringTypeName = 7;
public const int MethodToken = 2;
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting scenario to consider for situations where the inproc component stays the same while dotnet-monitor changes. I don't think we have to worry about that yet, but it seems like it would be possible to make this non-breaking change if we wanted to.

Copy link
Member

Choose a reason for hiding this comment

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

See #7974 (comment) -- this is an intention decision and inline with past decisions around this (#7493 (comment))

wiktork
wiktork previously approved these changes Mar 3, 2025
schmittjoseph
schmittjoseph previously approved these changes Mar 4, 2025
schmittjoseph
schmittjoseph previously approved these changes Mar 5, 2025
schmittjoseph
schmittjoseph previously approved these changes Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental-feature Pull requests that are focused on an experimental feature update-release-notes Pull requests that should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants