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 system.linux.memory.slab to system metrics #1078

Merged
merged 33 commits into from
Jul 29, 2024

Conversation

rogercoll
Copy link
Contributor

Fixes #531

Related to open-telemetry/opentelemetry-collector-contrib#7417

Changes

Adds the slab memory metric that is only available on Linux systems as system.linux.memory.slab.

Merge requirement checklist

@rogercoll rogercoll requested review from a team May 27, 2024 14:25
@rogercoll
Copy link
Contributor Author

For the attributes, it references the system.memory.state which will only match the total well-known value.
reclaimable and unreclaimable are additional custom state attributes normally associated with slab memory. For example, the current (0.101.0) version of the opentelemetry-collector-contrib uses system.memory.slab_reclaimable and system.memory.slab_unreclaimable to indicate those states.

Should this metric reference the system.memory.state attributes or create a new registry (e.g system.linux.memory.slab.state) that explicitly indicates the total, reclaimable and unreclaimable states?

@ChrsMark
Copy link
Member

ChrsMark commented May 28, 2024

Thank's @rogercoll for this PR!

Should this metric reference the system.memory.state attributes or create a new registry (e.g system.linux.memory.slab.state) that explicitly indicates the total, reclaimable and unreclaimable states?

I think it makes sense to have a specific *.state attribute which would include the reclaimable and unreclaimable.
As it was pointed out at #522 (from the usage naming convention):

Where appropriate, the sum of usage over all attribute values SHOULD be equal to the limit.

/cc @open-telemetry/semconv-system-approvers @braydonk

@mx-psi
Copy link
Member

mx-psi commented May 28, 2024

Agreed with @ChrsMark!

@rogercoll
Copy link
Contributor Author

I think it makes sense to have a specific *.state attribute which would include the reclaimable and unreclaimable.

Sounds good to me. To be aligned with the system.memory model, I think the following could work:

Metric name: system.linux.memory.slab.usage       
Attributes: system.linux.memory.slab.state (reclaimable and unreclaimable)
Metric name: system.linux.memory.slab.limit 

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

From SemConv perspective looks good to me.

One question: do we actually plan to implement the system.linux.memory.slab.limit as well in the Collector?

@rogercoll
Copy link
Contributor Author

One question: do we actually plan to implement the system.linux.memory.slab.limit as well in the Collector?

I would say so, the current sum of all system.memory.usage states do not equal to the limit which is needed for v1. If this PR moves forward, I can open an issue in the collector to track the implementation.

cc @dmitryax

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

model/registry/system.yaml Outdated Show resolved Hide resolved
model/metrics/system-metrics.yaml Outdated Show resolved Hide resolved
model/metrics/system-metrics.yaml Outdated Show resolved Hide resolved
model/metrics/system-metrics.yaml Outdated Show resolved Hide resolved
model/metrics/system-metrics.yaml Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jul 5, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 5, 2024
@github-actions github-actions bot removed the Stale label Jul 9, 2024
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Left a suggestion on attribute naming, look good otherwise!

docs/attributes-registry/linux.md Outdated Show resolved Hide resolved
@jsuereth jsuereth enabled auto-merge (squash) July 18, 2024 14:48
@jsuereth jsuereth disabled auto-merge July 18, 2024 14:48
@trisch-me
Copy link
Contributor

@rogercoll there are still some failing checks that prevent from merging

@trisch-me
Copy link
Contributor

@joaopgrassi could you check this one as you have requested changes but I think it's resolved now

@rogercoll
Copy link
Contributor Author

@rogercoll there are still some failing checks that prevent from merging

I think the failing checks are unrelated to the PR but to endpoints blocking the CI IP. Error message:

ERROR: 1 dead links found in ./docs/resource/k8s.md !
  [✖] https://www.itu.int/ITU-T/studygroups/com17/oid.html → Status: 0

I can reach the failed endpoint on my end.

@joaopgrassi joaopgrassi merged commit a44ff76 into open-telemetry:main Jul 29, 2024
12 checks passed
@rogercoll rogercoll deleted the add_linux_memory_slab branch July 29, 2024 15:37
maryliag pushed a commit to maryliag/semantic-conventions that referenced this pull request Jul 30, 2024
Co-authored-by: Liudmila Molkova <[email protected]>
Co-authored-by: Alexandra Konrad <[email protected]>
Co-authored-by: Joao Grassi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Proposal to add system.memory.slab
9 participants