Add a new module for extensions to the tracing APIs.#2962
Conversation
| * <p>Todo: figure out a better name for this. | ||
| */ | ||
| @SuppressWarnings("InconsistentOverloads") | ||
| public class Spanalyzer { |
There was a problem hiding this comment.
obviously, this is a ridiculous name. Suggestions extremely welcome.
There was a problem hiding this comment.
I know it was used in a different way in OpenTracing, but SpanWrapper kind of fits what you're doing here
There was a problem hiding this comment.
we also already have an internal class called SpanWrapper that could lead to confusion, but yes, this is one that I had considered.
There was a problem hiding this comment.
ExtendedTracer since presumably if moving these into core we'd add to Tracer?
There was a problem hiding this comment.
not bad. Or maybe TracerExtension?
There was a problem hiding this comment.
If kotlin, it'd definitely be TracerExtensions :) I guess per Java we'd want to name this what it is, which is an "extended tracer".
Codecov Report
@@ Coverage Diff @@
## main #2962 +/- ##
===========================================
+ Coverage 0 90.72% +90.72%
- Complexity 0 2820 +2820
===========================================
Files 0 325 +325
Lines 0 8816 +8816
Branches 0 885 +885
===========================================
+ Hits 0 7998 +7998
- Misses 0 553 +553
- Partials 0 265 +265
Continue to review full report at Codecov.
|
anuraaga
left a comment
There was a problem hiding this comment.
This seems reasonable to me
| * <p>Todo: figure out a better name for this. | ||
| */ | ||
| @SuppressWarnings("InconsistentOverloads") | ||
| public class Spanalyzer { |
There was a problem hiding this comment.
ExtendedTracer since presumably if moving these into core we'd add to Tracer?
|
@jkwatson By the way, did we want to call this package |
|
My 2 cents here: I think we should not move everything in core, but instead give users a stable extension artifact. This artifact may include helpers for metrics as well. |
anuraaga
left a comment
There was a problem hiding this comment.
Span creation is basically the whole point of OpenTelemetry - so I'm not a fan of splitting our own ecosystem if we keep this in an extension. "Are you using the extended tracer?", either when answering a support request or recommending how to use OpenTelemetry, isn't a question I'd like to have to ask in the long term. So for these, I would go with experimenting with the goal of getting them into core.
This isn't true of less-core features, for example an AttributeFilteringExporter has been asked for a few times I think - it has enough demand that it could probably be in core, but since it's a totally different concept than anything we have, it doesn't feel like a split to have it in an extension too.
anuraaga
left a comment
There was a problem hiding this comment.
Code LGTM - still wondering about extension vs incubator / long-term separation vs migrating into core. I do prefer incubator, but we could still do renames of the artifact after merging if that's better
oh, somehow I missed a rename to |
|
@jkwatson Yeah I think that'd be nice |
Do we want just |
|
@jkwatson Since API isn't meant to be split like the SDK is, |
|
Anything left on here to do? Would be nice to have this released as a part of 1.1.0 in a couple of weeks. |
Includes one simple possible example.
Includes one simple possible example.
Everything about this PR is up for modification. This is more of a proposal for some possible convenience APIs and a new module to house them.
I also realize that we could very easily add some kotlin extension functions to Tracer that use this, and that would probably be a nice enhancement for kotlin users.