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

Return consistent Meter for a given MeterProvider #1351

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

jtescher
Copy link
Member

@jtescher jtescher commented Nov 7, 2023

Changes

Adds a meter cache to return consistent Meter instances for the same scope. Meters have their own instrument caches so this avoids double instrument registration.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@jtescher jtescher requested a review from a team as a code owner November 7, 2023 21:57
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Files Coverage Δ
opentelemetry-sdk/src/metrics/meter_provider.rs 83.3% <86.6%> (+0.3%) ⬆️

📢 Thoughts on this report? Let us know!

@shaun-cox
Copy link
Contributor

@jtescher, I would kindly request we accept or reject #1328 before merging this, as otherwise I'll have merge conflicts to fix in #1328 I suspect... (and I've been waiting a day or so for final approval.) Thanks.

# Conflicts:
#	opentelemetry-sdk/src/metrics/meter_provider.rs

Meter::new(inst_provider)
if let Ok(mut meters) = self.meters.lock() {
Copy link
Member

Choose a reason for hiding this comment

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

Given the fact that obtaining meter is not free, we should mention in the documentation that, meter (and instruments) should be obtained once and cached by user to avoid perf hit. Not blocking this PR, but wanted to call it out here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can do some benchmarking in a follow up and have better guidelines on metric perf in general. The use case for accessing a meter is different than a tracer or logger as instruments are more likely to be a fixed set. And even in those cases repeatedly creating/accessing instruments could be the place that needs perf considerations.

# Conflicts:
#	opentelemetry-sdk/CHANGELOG.md
@jtescher jtescher merged commit 7bb97d5 into open-telemetry:main Nov 10, 2023
15 checks passed
@jtescher jtescher deleted the add-meter-cache branch November 10, 2023 20:11
jtescher added a commit to jtescher/opentelemetry-rust-1 that referenced this pull request Nov 13, 2023
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 this pull request may close these issues.

None yet

4 participants