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

[receiver/k8scluster] Do not keep metrics in memory #24769

Merged

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Aug 1, 2023

Construct metrics on every scrape instead of keeping them in memory and copy them with modifications on every scrape. We keep k8s objects in cache anyway, so we can use that instead of the pre-built metrics. This reduces RAM utilization.

This also allows us to extract a metrics builder instance and not create it on every scrape. This is the recommended approach that all other receivers follow. It ensures that any warnings defined in the metadata.yaml will be displayed only once, not on every scrape interval.

Memory utilization before/after the change on a cluster with 100 nodes and 1k pods:
Screenshot 2023-08-04 at 2 54 27 PM

@dmitryax dmitryax requested review from a team and djaglowski August 1, 2023 16:06
@dmitryax dmitryax changed the title [chore] [receiver/k8scluster] Reuse one MetricsBuilder instance [chore] [receiver/k8scluster] Reuse MetricsBuilder instance Aug 1, 2023
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch from bb66ede to be19f3c Compare August 1, 2023 16:16
@dmitryax dmitryax marked this pull request as draft August 2, 2023 05:59
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch from be19f3c to 7cb9c73 Compare August 2, 2023 08:17
@dmitryax dmitryax changed the title [chore] [receiver/k8scluster] Reuse MetricsBuilder instance [chore] [receiver/k8scluster] Do not keep all the metrics in memory Aug 2, 2023
@dmitryax dmitryax marked this pull request as ready for review August 2, 2023 08:17
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch 2 times, most recently from 1b17192 to 048b85d Compare August 2, 2023 08:53
@dmitryax dmitryax marked this pull request as draft August 2, 2023 09:47
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch 3 times, most recently from 61e2640 to b72ca92 Compare August 4, 2023 23:38
@dmitryax dmitryax marked this pull request as ready for review August 4, 2023 23:40
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch from b72ca92 to ade7882 Compare August 4, 2023 23:42
@dmitryax dmitryax changed the title [chore] [receiver/k8scluster] Do not keep all the metrics in memory [receiver/k8scluster] Do not keep metrics in memory Aug 4, 2023
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch from ade7882 to 08b9ba0 Compare August 4, 2023 23:57
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

Flies a bit over my head, but I know we have a good e2e test for this which would pick up regressions. LGTM

Construct metrics on every scrape instead. We keep k8s objects in cache anyway, so we can use that instead of the pre-built metrics. This reduces RAM utilization.

This also allows us extracting a metrics builder instance and do not create it on every scrape. This is the recommended approach that all other receivers follow. It ensures that any warnings defined in the metadata.yaml will be displayed only once, not on every scrape interval.
@dmitryax dmitryax force-pushed the k8scluster-move-metrics-builder branch from 08b9ba0 to fe8c6f3 Compare August 5, 2023 03:19
@dmitryax dmitryax merged commit 0fa58d2 into open-telemetry:main Aug 5, 2023
@github-actions github-actions bot added this to the next release milestone Aug 5, 2023
@dmitryax dmitryax deleted the k8scluster-move-metrics-builder branch August 5, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants