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

Recycle via reference counting #587

Merged
merged 8 commits into from
May 6, 2019

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Apr 16, 2019

  • Recycling is not done explicitly but implicitly when the reference count goes to 0
  • This also simplifies async context propagation
    • There's no need to copy the context to the other thread
    • The Span is just activated on the other thread
      If it's ended before the child starts it is still not recycled because the reference count is not at zero yet.
  • This is in also in preparation for breakdown metrics
    • You need to have a reference to the parent in order to calculate the self-time of spans
    • In order to be able to calculate the self-time in async scenarios, it's required to activate the span as opposed to just it's TraceContext

@felixbarny felixbarny mentioned this pull request Apr 16, 2019
8 tasks
@felixbarny felixbarny self-assigned this Apr 16, 2019
@codecov-io
Copy link

codecov-io commented Apr 16, 2019

Codecov Report

Merging #587 into master will decrease coverage by 0.36%.
The diff coverage is 64.1%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #587      +/-   ##
============================================
- Coverage      63.7%   63.34%   -0.37%     
  Complexity       68       68              
============================================
  Files           189      189              
  Lines          7431     7428       -3     
  Branches        871      861      -10     
============================================
- Hits           4734     4705      -29     
- Misses         2433     2444      +11     
- Partials        264      279      +15
Impacted Files Coverage Δ Complexity Δ
...m/agent/plugin/api/TransactionInstrumentation.java 52.38% <ø> (+7.14%) 0 <0> (ø) ⬇️
...agent/configuration/validation/RegexValidator.java 100% <ø> (ø) 0 <0> (ø) ⬇️
.../co/elastic/apm/agent/impl/payload/SystemInfo.java 79.61% <ø> (ø) 0 <0> (ø) ⬇️
...pm/agent/objectpool/impl/QueueBasedObjectPool.java 39.39% <ø> (ø) 0 <0> (ø) ⬇️
...c/apm/agent/impl/async/SpanInScopeBaseWrapper.java 62.96% <ø> (+4.62%) 0 <0> (ø) ⬇️
.../v6_4/ElasticsearchClientAsyncInstrumentation.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...pm/agent/configuration/converter/TimeDuration.java 90% <ø> (ø) 0 <0> (ø) ⬇️
...m/agent/objectpool/impl/ThreadLocalObjectPool.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...n/java/co/elastic/apm/agent/util/VersionUtils.java 60% <ø> (ø) 0 <0> (ø) ⬇️
...elastic/apm/agent/impl/payload/ProcessFactory.java 79.59% <ø> (ø) 0 <0> (ø) ⬇️
... and 199 more

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 17f6985...6075195. Read the comment docs.

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

I know this is a big PR already, but can we also lose TraceContextHolder and restore the simpler Span hierarchy now, or do we still need it?

@felixbarny felixbarny merged commit 3490077 into elastic:master May 6, 2019
@felixbarny felixbarny deleted the reference-counting branch May 6, 2019 15:56
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