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

profiler: add the Name field back for the CPU profile #1585

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

nsrip-dd
Copy link
Contributor

Accidentally removed by #1485

@nsrip-dd nsrip-dd requested a review from a team as a code owner November 17, 2022 13:00
@nsrip-dd nsrip-dd added this to the v1.44.0 milestone Nov 17, 2022
felixge
felixge previously approved these changes Nov 17, 2022
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

LGTM.

@nsrip-dd
Copy link
Contributor Author

nsrip-dd commented Nov 17, 2022

This doesn't seem to have been load bearing... The Name field is used to implement the String method for ProfileType, and that's only used in the startup log (explicitly) and when generating the profile type tag (implicitly, through a "%s" formatting string), which is in turn only used for some metrics. There was a test for the startup log output that was supposed to catch this, but didn't, so I updated the test to be a little more specific about the expected output.

Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

LGTM

@pr-commenter
Copy link

pr-commenter bot commented Nov 21, 2022

Benchmarks

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@nsrip-dd nsrip-dd merged commit 9b50352 into main Nov 21, 2022
@nsrip-dd nsrip-dd deleted the nick.ripley/add-back-cpu-profile-name branch November 21, 2022 13:54
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.

None yet

2 participants