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(sdk-metrics): prevent per-reader storages from keeping unreported accumulations in memory #4163

Merged
merged 13 commits into from
Oct 10, 2023

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Sep 26, 2023

Which problem is this PR solving?

As described in #4115 (reproducer https://github.com/pichlermarc/repro-4115), when two or more MetricReader instances are registered to a MeterProvider, it causes memory usage to increase until the app runs out of heap space.

This happens as TemporalMetricProcessor adds entries to a map _unreportedAccumulations for each MetricCollectorHandle for which there is one for each MetricReader. When one MetricReader collects, the unreported accumulation is first added to all the other MetricCollectorHandle entries. When the associated MetricCollectorHandle is never used with TemporalMetricsProcesssor#buildMetrics, its entry in _unreportedAccumulations is never reset - which causes the array for a MetricCollectorHandle to grow on each collection cycle until the app runs out of memory. This can happen when a SyncMetricStorage or AsyncMetricStorage is created and registered for a single MetricReader. In this case, MetricStorageRegistry#getMetricStorage will be called with a MetricCollectorHandle, only returning the storages with which the MetricReader is associated.

This never happened with a single MetricReader as every TemporalMetricsProcesssor ever registered will be collected in each cycle.

This PR fixes the issue by:

  • registering associated MetricCollectorHandles on creating the TemporalMetricsProcessor.
  • registering all MetricCollectorHandless for TemporalMetricsProcessors that are shared across all MetricReaders
  • registering only the single MetricCollectorHandle for TemporalMetricsProcessors that are intended for a single MetricReader.
  • not using the collectors argument from TemporalMetricsProcesssor#buildMetrics, but using the initially registered MetricCollectorHandle instead to ensure that only pre-registered MetricReaders that are expected to call TemporalMetricsProcesssor#buildMetrics eventually are kept in memory.

Fixes #4115

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #4163 (58fb183) into main (c320c98) will increase coverage by 0.46%.
The diff coverage is 94.11%.

❗ Current head 58fb183 differs from pull request most recent head d56ed32. Consider uploading reports for the commit d56ed32 to get more accurate results

@@            Coverage Diff             @@
##             main    #4163      +/-   ##
==========================================
+ Coverage   92.26%   92.72%   +0.46%     
==========================================
  Files         331      268      -63     
  Lines        9473     7907    -1566     
  Branches     1999     1682     -317     
==========================================
- Hits         8740     7332    -1408     
+ Misses        733      575     -158     
Files Coverage Δ
...ckages/sdk-metrics/src/state/AsyncMetricStorage.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/state/MeterSharedState.ts 96.61% <100.00%> (ø)
packages/sdk-metrics/src/state/MetricStorage.ts 100.00% <ø> (ø)
...ackages/sdk-metrics/src/state/SyncMetricStorage.ts 100.00% <100.00%> (ø)
...s/sdk-metrics/src/state/TemporalMetricProcessor.ts 95.23% <90.00%> (-3.07%) ⬇️

... and 71 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review September 26, 2023 12:38
@pichlermarc pichlermarc requested a review from a team September 26, 2023 12:38
@pichlermarc pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc labels Oct 5, 2023
@pichlermarc pichlermarc merged commit 4eb10f7 into open-telemetry:main Oct 10, 2023
16 checks passed
@pichlermarc pichlermarc deleted the fix/4115 branch October 10, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak when using more than one MetricReader
2 participants