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

HistogramMetric.record is not thread-safe #608

Open
angelo-ebay opened this issue Sep 27, 2024 · 1 comment · May be fixed by #609
Open

HistogramMetric.record is not thread-safe #608

angelo-ebay opened this issue Sep 27, 2024 · 1 comment · May be fixed by #609

Comments

@angelo-ebay
Copy link

angelo-ebay commented Sep 27, 2024

Hello OpenTelemetry friends, we've recently adopted OpenTelemetry metrics at eBay but we're experiencing an access race in HistogramMetricSdk.boundInstruments.

HistogramMetric.record is not thread-safe. Or, from an implementation perspective, HistogramMetricSdk.bind(labelSet:) is not thread safe.

MeterSdk reads boundInstruments of histograms during its collect implementation but it's possible for other threads to simultaneously write to boundInstruments while recording histogram values. This write occurs in HistogramMetricSdk.bind(labelSet:) when a new boundInstrument is added to the boundInstruments dictionary. MeterSdk uses collectLock to synchronize access to doubleHistograms but does not synchronize writes to the boundInstruments dictionary of histograms.

I've read that there is a new stable metrics implementation but the legacy implementation is fully functional so I'm wondering:

  • Are you open to accepting a PR to fix the synchronization issue in the legacy implementation although its replacement is in active development? I've noticed other instruments like as CounterMetricSdkBase have locks to synchronize access to boundInstruments
  • Is my better course of action to migrate to the new stable metrics implementation instead?
@nachoBonafonte
Copy link
Member

Migrating to the new metric implementation would be the best course of action, but we will accept any PR fixing existing functionality.

It mainly depends in the maturity of your current implementation; if you are starting with it, I would change to the new one, if you have it already working or almost working fix the issue and plan your transition.

@angelodipaolo angelodipaolo linked a pull request Sep 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants