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

Introduce TraceContextHolder, unifying context propagation #417

Merged
merged 6 commits into from
Jan 14, 2019

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Jan 11, 2019

Introducing TraceContextHolder in the internal API as a common
superclass of both TraceContext and AbstractSpan. Given an instance
of this class, you can create child spans, capture exceptions and manage
activations/scopes.

This abstraction reliefs clients from having to differ between the case
when the current activation is a TraceContext vs an AbstractSpan.

A TraceContext would be active when the current thread does not own
the lifecycle of the parent span. Otherwise an AbstractSpan would be
active.

There is rarely a case where a client would need to get the currently
active span to set the name or tags. This propagation is normally done
with @Advice.Local variables from enter to exit advices. Should the
need for that arise however, and the client can ensure that the current
activation is actually an AbstractSpan, all they need to do is to cast
tracer.getActive() from TraceContextHolder to AbstractSpan.

The public API does not differ between Span and TraceContextHolder.
ElasticApm.currentSpan() always returns a Span. In case the current
activation is a TraceContextHolder, methods like Span#setTag are
silent noops. Calling those methods on a Span whose lifecycle is not
owned by the caller is illegal anyway. However, calling
ElasticApm.currentSpan().createSpan() is always allowed.

Taking away ElasticApm.currentSpan() is not an option, as a common use
case is to rename the spans created by the auto instrumentation or to
add custom tags.

Also, ElasticApm.currentSpan().createSpan() makes for a much nicer and
more object-oriented API than starting a span on a tracer, passing in
the parent as a start option.

In preparation of #145

Introducing `TraceContextHolder` in the internal API as a common
superclass of both `TraceContext` and `AbstractSpan`. Given an instance
of this class, you can create child spans, capture exceptions and manage
activations/scopes.

This abstraction reliefs clients from having to differ between the case
when the current activation is a `TraceContext` vs an `AbstractSpan`.

A `TraceContext` would be active when the current thread does not own
the lifecycle of the parent span. Otherwise an `AbstractSpan` would be
active.

There is rarely a case where a client would need to get the currently
active span to set the name or tags. This propagation is normally done
with `@Advice.Local` variables from enter to exit advices. Should the
need for that arise however, and the client can ensure that the current
activation is actually an `AbstractSpan`, all they need to do is to cast
`tracer.getActive()` from `TraceContextHolder` to `AbstractSpan`.

The public API does not differ between `Span` and `TraceContextHolder`.
`ElasticApm.currentSpan()` always returns a `Span`. In case the current
activation is a `TraceContextHolder`, methods like `Span#setTag` are
silent noops. Calling those methods on a `Span` whose lifecycle is not
owned by the caller is illegal anyway. However, calling
`ElasticApm.currentSpan().createSpan()` is always allowed.

Taking away `ElasticApm.currentSpan()` is not an option, as a common use
case is to rename the spans created by the auto instrumentation or to
add custom tags.

Also, `ElasticApm.currentSpan().createSpan()` makes for a much nicer and
more object-oriented API than starting a span on a tracer, passing in
the parent as a start option.

In preparation of elastic#145
@felixbarny felixbarny requested a review from eyalkoren January 11, 2019 11:21
@codecov-io
Copy link

codecov-io commented Jan 11, 2019

Codecov Report

Merging #417 into master will increase coverage by 5.39%.
The diff coverage is 43.8%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #417      +/-   ##
===========================================
+ Coverage     66.31%   71.7%   +5.39%     
+ Complexity     1419    1315     -104     
===========================================
  Files           158     146      -12     
  Lines          6103    5104     -999     
  Branches        684     521     -163     
===========================================
- Hits           4047    3660     -387     
+ Misses         1797    1210     -587     
+ Partials        259     234      -25
Impacted Files Coverage Δ Complexity Δ
.../elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...src/main/java/co/elastic/apm/agent/impl/Scope.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...lastic/apm/agent/http/client/HttpClientHelper.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...astic/apm/agent/jdbc/StatementInstrumentation.java 55.55% <0%> (ø) 6 <0> (ø) ⬇️
...esttemplate/SpringRestTemplateInstrumentation.java 40.9% <0%> (ø) 4 <0> (ø) ⬇️
...nt/httpclient/ApacheHttpClientInstrumentation.java 44% <0%> (ø) 5 <0> (ø) ⬇️
...bci/methodmatching/TraceMethodInstrumentation.java 50% <0%> (+3.84%) 8 <0> (ø) ⬇️
...agent/plugin/api/ElasticApmApiInstrumentation.java 37.5% <0%> (ø) 3 <0> (ø) ⬇️
.../agent/plugin/api/AbstractSpanInstrumentation.java 50% <0%> (-4.26%) 3 <0> (-2)
...m/agent/plugin/api/CaptureSpanInstrumentation.java 41.66% <0%> (ø) 8 <0> (ø) ⬇️
... and 34 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 5beca3b...d0c3b5e. 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.

Nice! Makes a cleaner API. Few general comments:

  1. If I understand correctly, AbstractSpanImpl can now hold a ref to any TraceContextHolder - I understand that the name needs to stay span so not to not break API, but please change that in AbstractSpanImpl documentation and update the AbstractSpanInstrumentation to utilize the full benefits of this
  2. I saw you removed the binary serialization option. However, this PR is an infrastructure that enables activating a context in a different thread and we can't use the same instance due to recycling issues. Maybe use this PR to also pool TraceContexts that are used outside of Spans, so that when you want to activate a TraceContext on a different thread- take one from the pool and use copyFrom. Then we are allocation-free and still pool-safe.

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.

One very minor suggestion

@felixbarny
Copy link
Member Author

Thanks for the excellent review! 👌

@felixbarny felixbarny merged commit b1c30b6 into elastic:master Jan 14, 2019
@felixbarny felixbarny deleted the trace-context-holder branch January 14, 2019 13:12
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.

4 participants