Skip to content

Conversation

@lalitb
Copy link
Member

@lalitb lalitb commented Nov 22, 2024

Changes

  • std::sync::OnceLock was stabilized in Rust 1.70.0, providing equivalent functionality to once_cell::Lazy directly in the standard library. This change reduces the dependency on an external crate (once_cell) for a feature now natively supported.
  • Both once_cell::Lazy and std::sync::OnceLock guarantee thread-safe, one-time initialization of global variables. The replacement does not alter the functionality or behavior of the code.
  • Also unrelated change - pin-project-lite, futures-sink, futures-core, thiserror are made optional dependencies, and only used when trace feature is enabled. With this, opentelemetry crate doesn't use any external crate for logs and metrics.

Example of change for TracerProvider:
Before:

use once_cell::sync::Lazy;
use std::sync::{Arc, RwLock};

static GLOBAL_TRACER_PROVIDER: Lazy<RwLock<GlobalTracerProvider>> =
    Lazy::new(|| RwLock::new(GlobalTracerProvider::new(NoopTracerProvider::new())));

After

use std::sync::{OnceLock, RwLock};

static GLOBAL_TRACER_PROVIDER: OnceLock<RwLock<GlobalTracerProvider>> = OnceLock::new();

#[inline]
fn global_tracer_provider() -> &'static RwLock<GlobalTracerProvider> {
    GLOBAL_TRACER_PROVIDER
        .get_or_init(|| RwLock::new(GlobalTracerProvider::new(NoopTracerProvider::new())))
}

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)

@lalitb lalitb requested a review from a team as a code owner November 22, 2024 23:45
@lalitb lalitb changed the title Replace once_cell::Lazy with std::sync::OnceLock for Global Initialization in OpenTelemetry API crate Replace once_cell::Lazy with std::sync::OnceLock for global Initialization in OpenTelemetry API crate Nov 22, 2024
@codecov
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 58.62069% with 12 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (3d352d8) to head (00a9626).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry/src/global/propagation.rs 0.0% 12 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2326     +/-   ##
=======================================
- Coverage   79.5%   79.4%   -0.1%     
=======================================
  Files        123     123             
  Lines      21258   21271     +13     
=======================================
+ Hits       16905   16910      +5     
- Misses      4353    4361      +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks for driving to keep external dependencies minimal!

Lets merge after we get one more eyes to review.

Lazy::new(NoopTextMapPropagator::new);
static DEFAULT_TEXT_MAP_PROPAGATOR: OnceLock<NoopTextMapPropagator> = OnceLock::new();

/// Ensures the `GLOBAL_TEXT_MAP_PROPAGATOR` is initialized with a `NoopTextMapPropagator`.
Copy link
Contributor

@utpilla utpilla Nov 23, 2024

Choose a reason for hiding this comment

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

nit: For each of these methods, we could use a comment similar to what we have for Baggage:

/// Returns the default baggage, ensuring it is initialized only once.

So, something like:

/// Returns the global TextMapPropagator ensuring it is initialized only once.

@utpilla
Copy link
Contributor

utpilla commented Nov 23, 2024

Also unrelated change - pin-project-lite, futures-sink, futures-core, thiserror are made optional dependencies, and only used when trace feature is enabled.

We have trace added as a default feature. Do we want to change this to avoid adding these dependencies for the default build?

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

Left some non-blocking suggestions.

@lalitb
Copy link
Member Author

lalitb commented Nov 23, 2024

Also unrelated change - pin-project-lite, futures-sink, futures-core, thiserror are made optional dependencies, and only used when trace feature is enabled.

We have trace added as a default feature. Do we want to change this to avoid adding these dependencies for the default build?

Yes we can discuss this separate to this PR, as it will affect the trace consumers relying on the default build.

@cijothomas
Copy link
Member

Also unrelated change - pin-project-lite, futures-sink, futures-core, thiserror are made optional dependencies, and only used when trace feature is enabled.

We have trace added as a default feature. Do we want to change this to avoid adding these dependencies for the default build?

Yes we can discuss this separate to this PR, as it will affect the trace consumers relying on the default build.

For 1.0, it'd better to remove trace from default features, as we are not in a position to declare traces as stable. Lets track this separately.

@cijothomas cijothomas merged commit ebeeea1 into open-telemetry:main Nov 23, 2024
21 of 23 checks passed
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.

3 participants