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

Performance: By using const over static, improve noop span creation performance another 10% #1270

Merged
merged 1 commit into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions opentelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

- Bump MSRV to 1.64 [#1203](https://github.com/open-telemetry/opentelemetry-rust/pull/1203)
- `opentelemetry` crate now only carries the API types #1186. Use the `opentelemetry_sdk` crate for the SDK types.
- `trace::noop::NoopSpan` no longer implements `Default` and instead exposes
a `const DEFAULT` value. [#1270](https://github.com/open-telemetry/opentelemetry-rust/pull/1270)

## [v0.20.0](https://github.com/open-telemetry/opentelemetry-rust/compare/v0.19.0...v0.20.0)
This release should been seen as 1.0-rc3 following 1.0-rc2 in v0.19.0. Refer to CHANGELOG.md in individual creates for details on changes made in different creates.
Expand Down
7 changes: 3 additions & 4 deletions opentelemetry/src/trace/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use crate::{
};
use futures_core::stream::Stream;
use futures_sink::Sink;
use once_cell::sync::Lazy;
use pin_project_lite::pin_project;
use std::{
borrow::Cow,
Expand All @@ -16,10 +15,10 @@ use std::{
task::{Context as TaskContext, Poll},
};

static NOOP_SPAN: Lazy<SynchronizedSpan> = Lazy::new(|| SynchronizedSpan {
span_context: SpanContext::empty_context(),
const NOOP_SPAN: SynchronizedSpan = SynchronizedSpan {
span_context: SpanContext::NONE,
inner: None,
});
};

/// A reference to the currently active span in this context.
#[derive(Debug)]
Expand Down
34 changes: 8 additions & 26 deletions opentelemetry/src/trace/noop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
//! to have minimal resource utilization and runtime impact.
use crate::{
propagation::{text_map_propagator::FieldIter, Extractor, Injector, TextMapPropagator},
trace::{self, TraceContextExt, TraceFlags, TraceState},
trace::{self, TraceContextExt as _},
Context, InstrumentationLibrary, KeyValue,
};
use std::{borrow::Cow, sync::Arc, time::SystemTime};
Expand Down Expand Up @@ -38,25 +38,11 @@ pub struct NoopSpan {
span_context: trace::SpanContext,
}

impl Default for NoopSpan {
fn default() -> Self {
NoopSpan::new()
}
}

impl NoopSpan {
/// Creates a new `NoopSpan` instance.
pub fn new() -> Self {
NoopSpan {
span_context: trace::SpanContext::new(
trace::TraceId::INVALID,
trace::SpanId::INVALID,
TraceFlags::default(),
false,
TraceState::default(),
),
}
}
/// The default `NoopSpan`, as a constant
pub const DEFAULT: NoopSpan = NoopSpan {
djc marked this conversation as resolved.
Show resolved Hide resolved
span_context: trace::SpanContext::NONE,
};
}

impl trace::Span for NoopSpan {
Expand Down Expand Up @@ -135,12 +121,8 @@ impl trace::Tracer for NoopTracer {
/// If the span builder or the context's current span contains a valid span context, it is
/// propagated.
fn build_with_context(&self, _builder: trace::SpanBuilder, parent_cx: &Context) -> Self::Span {
if parent_cx.has_active_span() {
NoopSpan {
span_context: parent_cx.span().span_context().clone(),
}
} else {
NoopSpan::new()
NoopSpan {
span_context: parent_cx.span().span_context().clone(),
}
}
}
Expand Down Expand Up @@ -178,7 +160,7 @@ impl TextMapPropagator for NoopTextMapPropagator {
mod tests {
use super::*;
use crate::testing::trace::TestSpan;
use crate::trace::{self, Span, Tracer};
use crate::trace::{self, Span, TraceState, Tracer};

fn valid_span_context() -> trace::SpanContext {
trace::SpanContext::new(
Expand Down
28 changes: 21 additions & 7 deletions opentelemetry/src/trace/span_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ use thiserror::Error;
pub struct TraceFlags(u8);

impl TraceFlags {
/// Trace flags with the `sampled` flag set to `0`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Style suggestion: I usually like to put consts near the bottom instead of near the top, since they're usually relatively simple and don't change often.

///
/// Spans that are not sampled will be ignored by most tracing tools.
/// See the `sampled` section of the [W3C TraceContext specification] for details.
///
/// [W3C TraceContext specification]: https://www.w3.org/TR/trace-context/#sampled-flag
pub const NOT_SAMPLED: TraceFlags = TraceFlags(0x00);

/// Trace flags with the `sampled` flag set to `1`.
///
/// Spans that are not sampled will be ignored by most tracing tools.
Expand Down Expand Up @@ -216,6 +224,9 @@ impl fmt::LowerHex for SpanId {
pub struct TraceState(Option<VecDeque<(String, String)>>);

impl TraceState {
/// The default `TraceState`, as a constant
pub const NONE: TraceState = TraceState(None);

/// Validates that the given `TraceState` list-member key is valid per the [W3 Spec].
///
/// [W3 Spec]: https://www.w3.org/TR/trace-context/#key
Expand Down Expand Up @@ -457,15 +468,18 @@ pub struct SpanContext {
}

impl SpanContext {
/// An invalid span context
pub const NONE: SpanContext = SpanContext {
trace_id: TraceId::INVALID,
span_id: SpanId::INVALID,
trace_flags: TraceFlags::NOT_SAMPLED,
is_remote: false,
trace_state: TraceState::NONE,
};

/// Create an invalid empty span context
pub fn empty_context() -> Self {
SpanContext::new(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a lot of places using empty_context() that can be replaced with SpanContext::NONE. I haven't changed them in this PR, but my plan would be to change them all in a future PR and then remove the empty_context() function. Anyone see any problems with that approach?

TraceId::INVALID,
SpanId::INVALID,
TraceFlags::default(),
false,
TraceState::default(),
)
SpanContext::NONE
}

/// Construct a new `SpanContext`
Expand Down
Loading