-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[receiver/hostmetricsreceiver] Add system.memory.shared metric #45194
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
base: main
Are you sure you want to change the base?
[receiver/hostmetricsreceiver] Add system.memory.shared metric #45194
Conversation
This emits the system.memory.shared hostmetric via gopsutil, this was added to sem conv here https://github.com/open-telemetry/semantic-conventions/blob/main/docs/system/system-metrics.md#metric-systemmemoryshared
Currently gopsutil only supports shared memory on linux
Make generate was also ran and a memory scraper test was updated to match the new description
|
/rerun |
| on Linux systems due to platform-specific data availability. | ||
| This corresponds to the `Shmem` field in `/proc/meminfo`. | ||
| Support for other platforms may be added in the future when the underlying |
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.
This will only ever be available on Linux to my knowledge. To my knowledge, the other target platforms for the collector do not provide singular shared memory usage as a programatically readable statistic. I would recommend removing the suggestion that this will be supported by other platforms.
(We may also need to rename this metric in our semantic conventions to reflect that, CC @rogercoll)
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.
That makes sense, I have removed the suggestion that other platforms may support this in the future.
Regarding renaming, we could go with something like system.linux.memory.shared: That would be more accurate and consistent with system.linux.memory.available and system.linux.memory.dirty.
Should I wait for the semantic conventions to be updated first, or would you prefer I rename it in this PR and we update semconv afterward?
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.
We're having kind of a growing-pain with the OS name in semconv right now. Our written guidance suggests that the OS name goes after the "representative subsystem" of a metric namespace, and we would generally consider system.memory to be a "subsystem". So we would use the name system.memory.linux.shared in that case. The tension is that this name is kind of awkward; the "memory" is what's shared, not the "linux".
It's on my task list this week to get that sorted out once and for all. It's gonna come down to whether Semantic Conventions generally values clearer individual names or clearer organization within a namespace.
In the meantime, this PR can stay open since the functionality is all correct, and it's just down to the name.
Remove suggestion that the metric will be supported on other platforms in the future, as other platforms don't provide this statistic programmatically.
Description
Adding system.memory.shared metric to
receiver/hostmetricsreciever.Important
The utility gopsutil provides the shared memory value for Linux only atm. This PR has implemented this for Linux only.
Link to tracking issue
Closes #32712
Testing
Unit tests
Tests have been added and updated at
receiver/hostmetricsreciever.Local testing
make otelcontribcolsystem.memory.sharedOutput on Linux