Add getAttribute to ReadableSpan#3458
Conversation
|
|
||
| @Override | ||
| @Nullable | ||
| public <T> T getAttribute(AttributeKey<T> key) { |
There was a problem hiding this comment.
Except for hasEnded (write-once) all other getters are for read-only fields. From a theoretical perspective, I would prefer not to have this getter, if we can find another solution to the original problem on the issue.
There was a problem hiding this comment.
Because this is in the SDK and not API, I don't see a problem with adding these accessors if there's a use case. We actually have the toSpanData issue in zpages too just to getStatus.
Here the UX does seem significantly better for keeping attributes consistent within a trace without having to use more concepts such as Baggage.
Codecov Report
@@ Coverage Diff @@
## main #3458 +/- ##
============================================
- Coverage 90.66% 90.65% -0.02%
- Complexity 3237 3238 +1
============================================
Files 360 360
Lines 9978 9980 +2
Branches 1011 1012 +1
============================================
Hits 9047 9047
Misses 620 620
- Partials 311 313 +2
Continue to review full report at Codecov.
|
|
|
||
| @Override | ||
| @Nullable | ||
| public <T> T getAttribute(AttributeKey<T> key) { |
There was a problem hiding this comment.
Because this is in the SDK and not API, I don't see a problem with adding these accessors if there's a use case. We actually have the toSpanData issue in zpages too just to getStatus.
Here the UX does seem significantly better for keeping attributes consistent within a trace without having to use more concepts such as Baggage.
| */ | ||
| SpanKind getKind(); | ||
|
|
||
| /** Returns the value for the given {@link AttributeKey}, or {@code null} if not found. */ |
There was a problem hiding this comment.
I think it's worth a note here about the fact that this call does require some locking, and that subsequent calls may result in different results, due to the inherent mutability of the underlying object.
There was a problem hiding this comment.
do you want me to add that same note for hasEnded(), getName(), and getLatencyNanos() also?
There was a problem hiding this comment.
that's actually probably a good idea, yes, if you don't mind.
jkwatson
left a comment
There was a problem hiding this comment.
I'm ok with this, as long as we add some documentation about the performance implications of using it.
Resolves #3456