Support long, double and boolean for tags #234 #235
Conversation
| @@ -71,6 +71,21 @@ public Span tag(String key, String value) { | |||
| return this; | |||
| } | |||
|
|
|||
| @Override | |||
| public Span tag(String key, long value) { | |||
| return this; | |||
There was a problem hiding this comment.
I think that the default could be just converting this to strings. return tag(key, String.valueOf(value)). WDYT?
There was a problem hiding this comment.
This is good suggestion, I applied it for all Brave* classes because brave really don't support this. For noops I prefer to use shortest possible form
| @Override | ||
| public Span tag(String key, long value) { | ||
| this.delegate.setAttribute(key, value); | ||
| return new OtelSpan(this.delegate); |
There was a problem hiding this comment.
Come to think of it I wonder why wasn't I consistent here in comparison to other tracers. I'm creating a new OtelSpan back instead of returning this. 🤔
There was a problem hiding this comment.
I see this patter in multiple places, I don't think that it's really required, but hard to be sure.
nikoncode
force-pushed
the
wip-234-other-types-for-tags
branch
from
3decdd8
to
a88cd54
Compare
|
You need to run |
nikoncode
force-pushed
the
wip-234-other-types-for-tags
branch
3 times, most recently
from
58ce909
to
6adb7d7
Compare
| * @param value tag value | ||
| * @return this span | ||
| */ | ||
| Span tag(String key, long value); |
There was a problem hiding this comment.
I think we should add default here and provide a default that converts to String cause otherwise we're making a breaking change
| * @param value tag value | ||
| * @return this | ||
| */ | ||
| Builder tag(String key, long value); |
There was a problem hiding this comment.
This should be default and delegate to the String version. Otherwise, this is a breaking change
| * @param value tag value | ||
| * @return this, for chaining | ||
| */ | ||
| SpanCustomizer tag(String key, long value); |
There was a problem hiding this comment.
This should be default and delegate to the String version. Otherwise, this is a breaking change
nikoncode
force-pushed
the
wip-234-other-types-for-tags
branch
from
6adb7d7
to
8fc2f0d
Compare
|
@marcingrzejszczak I see that the whole library built to support only string, string pairs. My ultimate goal is to get support of these types from Convention layer, but I don't want to bloat this PR. I want to ask your opinion about:
|
nikoncode
force-pushed
the
wip-234-other-types-for-tags
branch
from
8fc2f0d
to
0faf66f
Compare
|
Done via #239 |
jonatan-ivanov
changed the title
jonatan-ivanov
added
the
superseded
This is only draft.
Brave doesn't support other types to be set as tags, so primitives will be converted into string (this is safe).
Will make it fancy, after some preliminary idea approval