opentelemetry-java icon indicating copy to clipboard operation
opentelemetry-java copied to clipboard

Use standard ArrayList size rather than max number of links for initial span links allocation

Open johnbley opened this issue 1 year ago • 5 comments

Most spans that have links don't grow to contain the maximum number of links [Citation needed]. So, when faulting in a links list to hold them, there's no need to assume we'll need memory to hold the maximum.

johnbley avatar Feb 26 '24 22:02 johnbley

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.88%. Comparing base (36bc703) to head (90713a0).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6252      +/-   ##
============================================
+ Coverage     90.86%   90.88%   +0.02%     
- Complexity     6030     6033       +3     
============================================
  Files           651      651              
  Lines         17728    17728              
  Branches       1777     1777              
============================================
+ Hits          16108    16112       +4     
+ Misses         1105     1103       -2     
+ Partials        515      513       -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 26 '24 22:02 codecov[bot]

  • no links (min)

For no links, this is faulted in, so any data structure's null is equally performant. I'm not sure I'd care to guess about the numerical distribution of any particular telemetry structure, much less tune to it. I'm pretty certain, however, that most spans don't reach the max number of links.

johnbley avatar Feb 27 '24 14:02 johnbley

I'd like to see before/after benchmarks for a variety of cases before changing this.

jkwatson avatar Feb 27 '24 16:02 jkwatson

I see where @trask and @jack-berg are coming from, and I don't want my ask to be a blocker. This is still probably a net improvement. I'm just a little hung up on the idea that a certain use case (messaging?) could generate lots of spans with ~11 links, which then requires one allocation for the first link and then another for the grow at 10. 🤷🏻

breedx-splk avatar Feb 27 '24 21:02 breedx-splk

generate lots of spans with ~11 links

Fair question. The maximal case I can think of is a lambda triggered by an sqs message, with xray links turned on, and somehow the http-level invocation also carries b3, jaeger, and opentracing headers configured to be attached as links, which doesn't get anywhere near 11 links. I grepped for addLinks and SpanLinksBuilder usage in opentelemetry-java-instrumentation and didn't see anything that operated in a loop or in batch.

johnbley avatar Feb 28 '24 13:02 johnbley

I also think this is a reasonable change. As for using a linked list, here is what Josh Block has to say about it https://twitter.com/joshbloch/status/583813919019573248?lang=en

laurit avatar May 06 '24 11:05 laurit