-
Notifications
You must be signed in to change notification settings - Fork 191
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
Fix units of process.open_file_descriptor.count and process.context_switches #1797
base: main
Are you sure you want to change the base?
Conversation
…witches Signed-off-by: ChrsMark <[email protected]>
I believe |
@@ -81,15 +81,15 @@ groups: | |||
stability: experimental | |||
brief: "Number of file descriptors in use by the process." | |||
instrument: updowncounter | |||
unit: "{count}" | |||
unit: "{open_file_descriptor}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe {descriptor}
or {file_descriptor}
?
I assume that if we had some other counter for file descriptors, it'd be right to use the same unit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answerwing with my original concern from the PR description :) :
I was not sure if it should be {open_file_descriptor} or just {file_descriptor} but went with what was proposed in #1662.
It makes sense to change it to file_descriptor
all together as @braydonk suggests at #1797 (comment).
I'm fine if we want to merge this one and iterate on it, or if we want to fix in a different PR entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this PR with the suggested change to {file_descriptor}
, I opened #1798 and if folks all agree we can make the metric name change as part of that.
@@ -81,15 +81,15 @@ groups: | |||
stability: experimental | |||
brief: "Number of file descriptors in use by the process." | |||
instrument: updowncounter | |||
unit: "{count}" | |||
unit: "{open_file_descriptor}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ +1
Fixes #1662
Changes
This PR fixes the units of
process.open_file_descriptor.count
andprocess.context_switches
metrics.I was not sure if it should be
{open_file_descriptor}
or just{file_descriptor}
but went with what was proposed in #1662.Merge requirement checklist
[chore]