Split SpanContext into interface / impl#1935
Conversation
anuraaga
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1935 +/- ##
============================================
- Coverage 85.04% 85.00% -0.05%
Complexity 2170 2170
============================================
Files 246 247 +1
Lines 8318 8320 +2
Branches 924 924
============================================
- Hits 7074 7072 -2
- Misses 907 910 +3
- Partials 337 338 +1
Continue to review full report at Codecov.
|
|
This seems to make the code more complicated and arguably does not implement the "SHOULD" in "The API MUST implement methods to create a SpanContext. These methods SHOULD be the only way to create a SpanContext." since you can now create a SpanContext by deriving from the interface and instantiate the derived class. |
|
|
||
| @Immutable | ||
| @AutoValue | ||
| abstract class ImmutableSpanContext implements SpanContext { |
There was a problem hiding this comment.
Since the interface is also annotated @Immutable and according to the spec any SpanContext must be Immutable, this is maybe not the best name to distinguish it from possible other implementations which would also be Immutable. Maybe DefaultSpanContext?
There was a problem hiding this comment.
Yeah I had the same impression - we have Immutable* as a pattern in many AutoValue implementations of interfaces currently. Renaming them all to Default seems fine to me, or even *Impl so the implementation orders next to the interface in the filetree.
There was a problem hiding this comment.
No strong opinion. I'd keep this name for this PR, and we can always rename later, since this is an internal detail.
|
@Oberon00 Weren't all the reasons in the long discussion in open-telemetry/opentelemetry-specification#969 and open-telemetry/opentelemetry-specification#970 ? :-) The short answer is for agent users, it will always be the agent's version of the API, not the user's version of the API, that instantiates a Also one point of note is that we don't lose any protection with this PR since it's already an abstract class. Of course, we could stop using AutoValue to prevent that, just an observation. With #1937 I am thinking of a general question on whether we should just avoid classes at all in the API - it's an API after all. If a custom SDK (which our auto instrumentation agent effectively is) finds a reason to override the implementation and satisfies a use case with it, why block it? |
|
I suspected as much 😃 But I thought it was better to document it on the PR that does the change. So the interface makes it even easier for the auto-instrumentation than the previous abstract class? |
|
@Oberon00 Yeah - one class can implement both the agent (shaded) API interface and application API interface, but with abstract classes we wouldn't be able to do that so end up copying between two classes instead. |
jkwatson
left a comment
jkwatson
left a comment
There was a problem hiding this comment.
If it helps auto-instrumentation and doesn't hurt the end-user experience, I'm all in favor.
| @@ -17,23 +15,15 @@ | |||
| * traceState} and the {@link boolean remote} flag. | |||
| */ | |||
| @Immutable | |||
There was a problem hiding this comment.
Does this still make sense on an interface?
There was a problem hiding this comment.
I think it does. It places an requirement on all implementers and offers guarantees to users.
There was a problem hiding this comment.
Obviously, it's a soft requirement, since it can't be enforced, but I still think it's a good thing to document.
|
After further thinking, I'd prefer SpanContext stay not as an interface, but as a final class (or AutoValue'd class). It's a core piece of data for the API, and since it's data-only, having it be non-interface is preferable to me. |
|
@bogdandrutu can you unblock this PR, or comment on #1946 which you blocked this on? |
Note: although I usually favor data-as-classes, I'm on board with this change. |
| default boolean isValid() { | ||
| return TraceId.isValid(getTraceIdAsHexString()) && SpanId.isValid(getSpanIdAsHexString()); | ||
| } |
There was a problem hiding this comment.
this will become more expensive after this change, we should calculate this only once since it is used couple of times per instance created.
There was a problem hiding this comment.
Also, see above...this is still memoized by the implementation.
| default byte[] getSpanIdBytes() { | ||
| return SpanId.bytesFromHex(getSpanIdAsHexString(), 0); | ||
| } |
There was a problem hiding this comment.
Why removing the ability to memoized this call? It may get expensive. I would suggest to not default this and any method that was memoized before.
There was a problem hiding this comment.
This is still memoized by the autovalue implementation. See ImmutableSpanContext above.
|
My concern still stays on a general level. If there was no auto-instrumentation project majority of the engineers have implemented this as a final class, so I am concerned that we do designed choices based on an external project need that may influence the overall usability and performance of the core product. This designed choices may affect us in the future in an unknown way. I do understand the needs coming from auto-instrumentation, but you need to keep in mind that right now we need to be prepared to deal with cases where users will implement this in a "unexpected" way like having mutable objects inside the SpanContext, etc. this may cause problems for us, and we may have to deal with this. |
bogdandrutu
dismissed
their stale review
Discussed with @jkwatson, we will add a note about not supporting alternative implementations
… into spancontext-interface
|
Thanks! Added a warning |
|
@marcingrzejszczak this is potentially a breaking change |
6 participants
As enabled by open-telemetry/opentelemetry-specification#969