Skip to content

InternalLog fixes for GlobalMeterProvider #2333

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 24, 2024
Merged

InternalLog fixes for GlobalMeterProvider #2333

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
969dc65
InternalLog fixes for GlobalMeterProvider
cijothomas Nov 24, 2024
687dbfb
Merge branch 'main' into cijothomas/internallogs4
cijothomas Nov 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions opentelemetry/src/global/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,24 @@
{
// Try to set the global meter provider. If the RwLock is poisoned, we'll log an error.
let mut global_provider = global_meter_provider().write();

if let Ok(ref mut provider) = global_provider {
**provider = Arc::new(new_provider);
otel_info!(name: "MeterProvider.GlobalSet", message = "Global meter provider is set. Meters can now be created using global::meter() or global::meter_with_scope().");
} else {
otel_error!(name: "MeterProvider.GlobalSetFailed", message = "Global meter provider is not set due to lock poison. Meters created using global::meter() or global::meter_with_scope() will not function.");
otel_error!(name: "MeterProvider.GlobalSetFailed", message = "Setting global meter provider failed. Meters created using global::meter() or global::meter_with_scope() will not function. Report this issue in OpenTelemetry repo.");

Check warning on line 28 in opentelemetry/src/global/metrics.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/global/metrics.rs#L28 

Added line #L28 was not covered by tests
Copy link
Member Author

@cijothomas cijothomas Nov 24, 2024

Choose a reason for hiding this comment

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

in a future PR, i'll try to make this consistent, by providing an attribute "suggested_action" where the value can inform users about actions they can do. In this case there are 2

  1. Report it at OpenTelemetry repro.
  2. Use meters from other meter provider instance, if they have access to.

This requires some time to come up with a consistent way across entire repo to ensure every warn/err logs have a clear action for end users.

}
}

/// Returns an instance of the currently configured global [`MeterProvider`].
pub fn meter_provider() -> GlobalMeterProvider {
global_meter_provider()
.read()
.expect("GLOBAL_METER_PROVIDER RwLock poisoned")
.clone()
// Try to get the global meter provider. If the RwLock is poisoned, we'll log an error and return a NoopMeterProvider.
let global_provider = global_meter_provider().read();
if let Ok(provider) = global_provider {
provider.clone()
} else {
otel_error!(name: "MeterProvider.GlobalGetFailed", message = "Getting global meter provider failed. Meters created using global::meter() or global::meter_with_scope() will not function. Report this issue in OpenTelemetry repo.");
Arc::new(crate::metrics::noop::NoopMeterProvider::new())

Check warning on line 40 in opentelemetry/src/global/metrics.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/global/metrics.rs#L39-L40 

Added lines #L39 - L40 were not covered by tests
Copy link
Member

@lalitb lalitb Nov 24, 2024

Choose a reason for hiding this comment

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

This does great, to maintain continuity for application. Additionally, we can also consider trying to recover the poisoned provider, check if it's still intact (since corruption could happen if another thread panicked while holding the lock), and return it if everything is good. Of course, this would need to be handled on a case-by-case basis, and only where recovery makes sense. Something, to be tried separate of this PR, I will have a look if it is possible.

Copy link
Member Author

@cijothomas cijothomas Nov 24, 2024

Choose a reason for hiding this comment

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

agree. we absolutely cannot panic here so this is the minimal change to meet that goal. lets see if we can recover nicely.

}
}

/// Creates a named [`Meter`] via the currently configured global [`MeterProvider`].
Expand Down
Loading