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

SpanLimits going over limit set for max events per spans #1148

Closed
wperron opened this issue Jul 13, 2023 · 3 comments · Fixed by #1151
Closed

SpanLimits going over limit set for max events per spans #1148

wperron opened this issue Jul 13, 2023 · 3 comments · Fixed by #1151
Labels
A-trace Area: issues related to tracing bug Something isn't working

Comments

@wperron
Copy link
Contributor

wperron commented Jul 13, 2023

I'm trying to set the limit on events per spans to 0, but events are still being exported, bypassing the limit set in the config. Here's a minimal repro:

/// Run with `OTEL_SPAN_EVENT_COUNT_LIMIT=0 cargo run`

use opentelemetry::{
    global,
    sdk::{self, export::trace::stdout},
    trace::{Span, Tracer},
};

fn main() {
    // Create a new trace pipeline that prints to stdout
    let tracer = stdout::new_pipeline()
        .with_trace_config(sdk::trace::config())
        .install_simple();

    {
        let mut span = tracer.start("doing work");
        span.add_event("my event", vec![]);
        span.add_event("my other event", vec![]);
    }

    // Shutdown trace pipeline
    global::shutdown_tracer_provider();
}

Outputs:

SpanData { span_context: SpanContext { trace_id: 681f5965702c75e41724caf5b33b9d1b, span_id: f682e85763f26d16, trace_flags: TraceFlags(1), is_remote: false, trace_state: TraceState(None) }, parent_span_id: 0000000000000000, span_kind: Internal, name: "doing work", start_time: SystemTime { tv_sec: 1689254989, tv_nsec: 766169000 }, end_time: SystemTime { tv_sec: 1689254989, tv_nsec: 766173000 }, attributes: EvictedHashMap { map: {}, evict_list: [], max_len: 128, dropped_count: 0 }, events: EvictedQueue { queue: Some([Event { name: "my event", timestamp: SystemTime { tv_sec: 1689254989, tv_nsec: 766172000 }, attributes: [KeyValue { key: Static("my_attribute"), value: String(Static("some_value")) }], dropped_attributes_count: 0 }]), max_len: 0, dropped_count: 1 }, links: EvictedQueue { queue: None, max_len: 128, dropped_count: 0 }, status: Unset, resource: Resource { attrs: {Static("service.name"): String(Owned("unknown_service"))}, schema_url: None }, instrumentation_lib: InstrumentationLibrary { name: "opentelemetry", version: Some("0.19.0"), schema_url: None } }

I thought this might be an edge case for 0 and tried setting the limit to 1 but the same issue appears. I also tried overwriting the limit in code rather than through the environment variable, no luck.

I haven't looked to see if this also applied to the other limits as well.


Edit: This also has a negative side effect when setting events with SpanBuilder, if the limit is set to 0 in that case, all the events are attached to the EvictedQueue, basically putting no limit on the number of events or links on a span.

let sb = tracer.span_builder("doing work").with_events(vec![
    Event::new("my event", SystemTime::now(), vec![], 0),
    Event::new("my second event", SystemTime::now(), vec![], 0),
    Event::new("my third event", SystemTime::now(), vec![], 0),
]);
let _ = tracer.build_with_context(sb, &Context::current());

This particularly affects tracing-opentelemetry since that's how they create OpenTelemetry spans from their spans.

@wperron
Copy link
Contributor Author

wperron commented Jul 13, 2023

Just verified all the limit options:

  • ✅ max_attributes_per_event
  • ✅ max_attributes_per_link
  • ✅ max_attributes_per_span
  • 🚫 max_events_per_span
  • 🚫 max_links_per_span

@wperron
Copy link
Contributor Author

wperron commented Jul 14, 2023

Investigating this further, I think this is a bug with EvictedQueue -- all the max_attributes_* limit use an EvictedHashMap and don't exhibit the issue, while max_events_per_span and max_links_per_span use EvictedQueue and do exhibit the issue.

wperron added a commit to wperron/opentelemetry-rust that referenced this issue Jul 14, 2023
The EvictedQueue was checking for the length _before_ inserting, and
popping extra items, then doing the insertion. In the case where the
capacity is set to zero, it caused the pop operation to be a no-op on
the first insert, and then insert an item anyway.

This commit fixes the issue by moving the length check after the insert
and popping any extra items.

Fixes open-telemetry#1148
wperron added a commit to wperron/opentelemetry-rust that referenced this issue Jul 14, 2023
The EvictedQueue was checking for the length _before_ inserting, and
popping extra items, then doing the insertion. In the case where the
capacity is set to zero, it caused the pop operation to be a no-op on
the first insert, and then insert an item anyway.

This commit fixes the issue by moving the length check after the insert
and popping any extra items.

Fixes open-telemetry#1148
@TommyCpp TommyCpp added bug Something isn't working A-trace Area: issues related to tracing labels Jul 16, 2023
@cijothomas
Copy link
Member

@wperron Can you describe more what is the use case for setting the limit to 0 for span events? It is mentioned this is needed for tracing-opentelemetry - but not clear.. If the intention is not to send tracing events as Otel Span Events, then tracing-opentelemetry can provide a bool flag to control that, right?

If you can comment on #1283 (comment), that'd help while working on some improvements (including potentially removing this feature) in the area!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trace Area: issues related to tracing bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants