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

Optimize SpanBuilderSdk link memory #753

Merged

Conversation

jkwatson
Copy link
Contributor

@jkwatson jkwatson commented Jan 14, 2020

resolves #709

@codecov-io
Copy link

codecov-io commented Jan 14, 2020

Codecov Report

Merging #753 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #753      +/-   ##
============================================
- Coverage     78.84%   78.79%   -0.06%     
+ Complexity      761      759       -2     
============================================
  Files            98       98              
  Lines          2709     2711       +2     
  Branches        255      256       +1     
============================================
  Hits           2136     2136              
- Misses          473      474       +1     
- Partials        100      101       +1
Impacted Files Coverage Δ Complexity Δ
...main/java/io/opentelemetry/sdk/trace/SpanData.java 100% <100%> (ø) 2 <0> (ø) ⬇️
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java 93.75% <100%> (-0.07%) 34 <0> (-1)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java 90.24% <0%> (-1.22%) 43% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5517dcb...cf86f36. Read the comment docs.

@carlosalberto
Copy link
Contributor

This looks fine to me - and while on that subject, I've long wondered if we should keep the same behavior we have with Attributes, dropping the eldest entry once we have reached the maximum allowed count.

Maybe @bogdandrutu remembers about this, since we are using OC based code? ;)

@jkwatson
Copy link
Contributor Author

Do you know why we would drop the "eldest" entry, rather than just dropping any that exceed the maximum? Is there a reason why recency would mean more relevance?

@jkwatson jkwatson force-pushed the optimize_span_link_memory branch from 098a2fd to cf86f36 Compare January 14, 2020 22:49
@jkwatson jkwatson changed the title Optimize span link memory Optimize SpanBuilderSdk link memory Jan 15, 2020
@@ -127,10 +128,18 @@
@Override
public Span.Builder addLink(Link link) {
Utils.checkNotNull(link, "link");
totalNumberOfLinksAdded++;
// don't bother doing anything with any links beyond the max.
Copy link
Member

Choose a reason for hiding this comment

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

It is better to drop the earliest in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why? Are later links somehow better than earlier ones? What's the business rationale here? [the code is much more efficient if we can just drop the later ones]

@bogdandrutu bogdandrutu merged commit 564a323 into open-telemetry:master Jan 17, 2020
@jkwatson jkwatson deleted the optimize_span_link_memory branch February 28, 2020 21:31
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.

SpanBuilderSdk.truncatedLinks may keep references to thrown away links
4 participants