[processor/spanmetrics] Prune histograms #27083

nijave · 2023-09-23T01:07:49Z

Prune histograms when the dimension cache evictions are removed

Description:
Prunes histograms when the dimension cache is pruned. This prevents metric series from growing indefinitely

Link to tracking Issue:
#27080

Testing:
I modified the the existing test to check histograms length instead of dimensions cache length. This required simulating ticks to hit the exportMetrics function

Documentation:

linux-foundation-easycla · 2023-09-23T01:07:52Z

The committers listed above are authorized under a signed CLA.

✅ login: nijave / name: Nick Venenga (926f2a3)
✅ login: MovieStoreGuy / name: Sean Marciniak (3745bed)

albertteoh

Thanks @nijave!

Please be advised though that the spanmetricsprocessor is deprecated.

I believe the spanmetricsconnector does prune histograms, however, please let me know if I've misunderstood something.

mfilipe · 2023-09-25T21:12:16Z

Thanks @nijave!

Please be advised though that the spanmetricsprocessor is deprecated.

I believe the spanmetricsconnector does prune histograms, however, please let me know if I've misunderstood something.

@albertteoh I don't think it is being pruned.

MovieStoreGuy · 2023-09-30T14:17:21Z

Ahh, if this is related to memory management and concerns, then performing a delete on a map doesn't release the underlying storage until map is scheduled for GC.

The easiest way to address this is use pointer values for the value type since it keeps the memory allocated small.

lastly, this is a deprecated component you're trying to update. Could you please apply the fix to the connector version instead?

nijave · 2023-09-30T17:11:29Z

Ahh, if this is related to memory management and concerns, then performing a delete on a map doesn't release the underlying storage until map is scheduled for GC.

The easiest way to address this is use pointer values for the value type since it keeps the memory allocated small.

lastly, this is a deprecated component you're trying to update. Could you please apply the fix to the connector version instead?

It's less about memory management and more about controlling Prometheus active series which get emitted to long term storage. We're seeing about an extra ~1m active series pile up which is $8k/mon on Grafana Cloud. Actually, the original workaround was to set Grafana Agent to have a 2GiB memory limit on Kubernetes and let it get OOMKill'd before it could pile up too many metrics.

Usually what's happening for us, some errors come in and generate new series. They drop off but the series stick around until the agents are restarted which can be weeks or even months. We also have batch jobs and other periodic workloads that emit trace data occasionally but not regularly and these pile up as well.

We're using Grafana Agent which pulls this in as a library and we need to rewrite/test our config using their new config pattern if we want to use connector so, for us, we need to update our deprecated Grafana Agent config first.

If I have time, I'll take a look at the connector and see if it has the same problem. I'm actually switching jobs so after Oct 6th, I won't have a real environment available to test changes anymore.

nijave · 2023-09-30T17:14:32Z

Ahh, if this is related to memory management and concerns, then performing a delete on a map doesn't release the underlying storage until map is scheduled for GC.

That is interesting--I'm not very well versed in go (especially the GC quirks) so I'll have to do some reading up but I suspect that's not an issue for the use case I described above.

MovieStoreGuy · 2023-10-03T00:12:52Z

@nijave I've rerun the tests, could I ask you to address any failures and provide a changelog?

Considering this is related to the Grafana, I'll cc @jpkrohling to see if they can also address this upstream as well :)

nijave · 2023-10-03T22:28:53Z

@nijave I've rerun the tests, could I ask you to address any failures and provide a changelog?

Considering this is related to the Grafana, I'll cc @jpkrohling to see if they can also address this upstream as well :)

Ok, I think I addressed all the issues.

I have grafana/agent#5271 open on Grafana Agent (I think it's linked in the corresponding issue in this repo but not on this PR)

Prune histograms when the dimension cache evictions are removed

nijave · 2023-10-04T00:51:11Z

.chloggen/27083-spanmetrics-prune.yaml

+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix


Not really sure about this one. Technically this will produce less metric series than it previously did (the output of this component is changing) but my understanding was it was always intended that metric series (histograms) get pruned at the same time as dimensions cache map.

i.e. it was a bug these histograms weren't being pruned

Prune histograms when the dimension cache evictions are removed **Description:** Prunes histograms when the dimension cache is pruned. This prevents metric series from growing indefinitely **Link to tracking Issue:** open-telemetry#27080 **Testing:** I modified the the existing test to check `histograms` length instead of dimensions cache length. This required simulating ticks to hit the exportMetrics function **Documentation:** <Describe the documentation added.> Co-authored-by: Sean Marciniak <[email protected]>

nijave · 2023-10-06T13:00:04Z

Here's some numbers after running in our dev environment for a day with this change grafana/agent#5271 (comment)

Prune histograms when the dimension cache evictions are removed **Description:** Prunes histograms when the dimension cache is pruned. This prevents metric series from growing indefinitely **Link to tracking Issue:** open-telemetry#27080 **Testing:** I modified the the existing test to check `histograms` length instead of dimensions cache length. This required simulating ticks to hit the exportMetrics function **Documentation:** <Describe the documentation added.> Co-authored-by: Sean Marciniak <[email protected]>

nijave requested review from a team and dashpole September 23, 2023 01:07

github-actions bot assigned MovieStoreGuy Sep 23, 2023

github-actions bot added the processor/spanmetrics Span Metrics processor label Sep 23, 2023

github-actions bot requested a review from albertteoh September 23, 2023 01:08

nijave force-pushed the nv-prune-spanmetrics-histograms branch from 1150f37 to 70daeff Compare September 23, 2023 01:12

nijave mentioned this pull request Sep 23, 2023

spanmetricsprocessor doesn't prune histograms when metric cache is pruned #27080

Closed

nijave force-pushed the nv-prune-spanmetrics-histograms branch from 70daeff to 9375a88 Compare September 23, 2023 01:21

albertteoh approved these changes Sep 24, 2023

View reviewed changes

MovieStoreGuy approved these changes Sep 30, 2023

View reviewed changes

nijave force-pushed the nv-prune-spanmetrics-histograms branch 5 times, most recently from 1b3417a to 5633c3f Compare October 3, 2023 22:23

nijave changed the title ~~Prune spanmetrics histograms~~ [processor/spanmetrics] Prune histograms Oct 3, 2023

nijave force-pushed the nv-prune-spanmetrics-histograms branch from 5633c3f to 5946572 Compare October 3, 2023 22:27

[processor/spanmetrics] Prune histograms

926f2a3

Prune histograms when the dimension cache evictions are removed

nijave force-pushed the nv-prune-spanmetrics-histograms branch from 5946572 to 926f2a3 Compare October 3, 2023 22:47

Merge branch 'main' into nv-prune-spanmetrics-histograms

3745bed

nijave commented Oct 4, 2023

View reviewed changes

MovieStoreGuy merged commit 6aac9bd into open-telemetry:main Oct 4, 2023
87 checks passed

github-actions bot added this to the next release milestone Oct 4, 2023

nijave deleted the nv-prune-spanmetrics-histograms branch October 4, 2023 12:05

nijave mentioned this pull request Oct 6, 2023

spanmetrics grow indefinitely grafana/agent#5271

Closed

ptodev mentioned this pull request Oct 12, 2023

Upgrade OpenTelemetry Collector to at least v0.87.0 grafana/agent#5467

Closed

nijave mentioned this pull request Dec 20, 2023

[exporter/prometheus] Expired metrics were not be deleted #17306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[processor/spanmetrics] Prune histograms #27083

[processor/spanmetrics] Prune histograms #27083

nijave commented Sep 23, 2023

linux-foundation-easycla bot commented Sep 23, 2023 •

edited

Loading

albertteoh left a comment

mfilipe commented Sep 25, 2023

MovieStoreGuy commented Sep 30, 2023

nijave commented Sep 30, 2023

nijave commented Sep 30, 2023

MovieStoreGuy commented Oct 3, 2023

nijave commented Oct 3, 2023 •

edited

Loading

nijave Oct 4, 2023 •

edited

Loading

nijave commented Oct 6, 2023

[processor/spanmetrics] Prune histograms #27083

[processor/spanmetrics] Prune histograms #27083

Conversation

nijave commented Sep 23, 2023

linux-foundation-easycla bot commented Sep 23, 2023 • edited Loading

albertteoh left a comment

Choose a reason for hiding this comment

mfilipe commented Sep 25, 2023

MovieStoreGuy commented Sep 30, 2023

nijave commented Sep 30, 2023

nijave commented Sep 30, 2023

MovieStoreGuy commented Oct 3, 2023

nijave commented Oct 3, 2023 • edited Loading

nijave Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

nijave commented Oct 6, 2023

linux-foundation-easycla bot commented Sep 23, 2023 •

edited

Loading

nijave commented Oct 3, 2023 •

edited

Loading

nijave Oct 4, 2023 •

edited

Loading