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

fix(hostmetricsreceiver): do not duplicate mountpoint metrics #34635

Merged
merged 6 commits into from
Oct 9, 2024

Conversation

tcolgate
Copy link
Contributor

Mountpoints can be reported multiple times for each mount into a namespace. This causes duplicate metrics which causes issues with some exporters. Each instance of the mountpoint will have identical metrics, so it is safe to ignore repeated mountpoints.

Closes #34512

Description:

Link to tracking Issue:

Testing:

Documentation:

Mountpoints can be reported multiple times for each mount into a
namespace. This causes duplicate metrics which causes issues with
some exporters. Each instance of the mountpoint will have identical
metrics, so it is safe to ignore repeated mountpoints.

Closes open-telemetry#34512
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question before I approve.

mountpoint: partition.Mountpoint,
device: partition.Device,
}
if _, ok := seen[key]; partition.Mountpoint != "" && ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify to me why the check for Mountpoint being an empty string is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "Virtual partitions" test fails without that as virtual partitions don't have mount points. See test Include device filtering that includes virtual partitions (I'm not really convinced about the validity of that particular feature/test, you get mount entries without devices, where only a filesystem type like procfs is used, but they do get mounted somewhere)

@atoulme
Copy link
Contributor

atoulme commented Aug 15, 2024

Please add a changelog.

@tcolgate tcolgate requested a review from braydonk August 20, 2024 09:44
Copy link
Contributor

github-actions bot commented Sep 4, 2024

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

@github-actions github-actions bot added the Stale label Sep 4, 2024
@tcolgate
Copy link
Contributor Author

tcolgate commented Sep 4, 2024

@atoulme I added a change log a while back, any chance of a re-review?

@atoulme atoulme removed the Stale label Sep 4, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

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

@github-actions github-actions bot added the Stale label Oct 3, 2024
@tcolgate tcolgate requested a review from a team as a code owner October 3, 2024 10:46
@tcolgate
Copy link
Contributor Author

tcolgate commented Oct 3, 2024

@atoulme the checks didn't run last time this was ack'd, so I couldn't merge.

Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Approved on behalf of codeowners, but someone with the right access will have to approve the test workflows.

@tcolgate
Copy link
Contributor Author

tcolgate commented Oct 3, 2024

@braydonk how can I make progress on this? Can you atleast remove the stale flag? Also, by the time anyone approves, I seem to need to end up rebasing/updating the branch, which then seems to need the tests re-run, so I feel a bit stuck. Am I doing something wrong, or do I just need to find someone "on the inside", to help this along?

@braydonk
Copy link
Contributor

braydonk commented Oct 3, 2024

I do not have access to remove the Stale label either. Once the workflows are approved they should be able to run without re-approval so that should help with that problem.

However, according to the requirements this isn't fully approved even if the tests were to run. We still need approval from someone in @open-telemetry/collector-contrib-approvers. Someone from that group should have access, so since I've @'d hopefully one of them sees and does the things that require access here (remove Stale label, approve workflows, approve PR, merge).

@dashpole dashpole removed the Stale label Oct 3, 2024
@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Oct 3, 2024
@andrzej-stencel andrzej-stencel merged commit a2b67d9 into open-telemetry:main Oct 9, 2024
167 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 9, 2024
jmichalek132 pushed a commit to jmichalek132/opentelemetry-collector-contrib that referenced this pull request Oct 10, 2024
…elemetry#34635)

Mountpoints can be reported multiple times for each mount into a
namespace. This causes duplicate metrics which causes issues with some
exporters. Each instance of the mountpoint will have identical metrics,
so it is safe to ignore repeated mountpoints.

Closes open-telemetry#34512

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…elemetry#34635)

Mountpoints can be reported multiple times for each mount into a
namespace. This causes duplicate metrics which causes issues with some
exporters. Each instance of the mountpoint will have identical metrics,
so it is safe to ignore repeated mountpoints.

Closes open-telemetry#34512

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/hostmetrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hostmetrics receiver duplicates filesystem metrics on GKE
6 participants