Adjust tag naming for better consistency #140
Conversation
tylerbenson
force-pushed
the
tyler/consistency
branch
from
aab6cfa
to
7f8b814
Compare
references to entries below were using the wrong instance and thus not being cleaned properly.
| public static final String THREAD_NAME = "thread-name"; | ||
| public static final String THREAD_ID = "thread-id"; | ||
| public static final String SPAN_TYPE = "span.type"; | ||
| public static final String SERVICE_NAME = "service.name"; |
There was a problem hiding this comment.
That is going to break most of Java instrumentation. We should provide a migration guide in the release note and notify our beta customers about this breaking change. But yea, I prefer the dot notation too.
| @@ -89,7 +90,7 @@ class DDApiTest extends Specification { | |||
| [SpanFactory.newSpanOf(1L)] | [new TreeMap<>([ | |||
| "duration" : 0, | |||
| "error" : 0, | |||
| "meta" : ["thread-name": Thread.currentThread().getName(), "thread-id": "${Thread.currentThread().id}"], | |||
| "meta" : [(DDTags.THREAD_NAME): Thread.currentThread().getName(), (DDTags.THREAD_ID): "${Thread.currentThread().id}"], | |||
There was a problem hiding this comment.
Same here?
Probably there are more places where we need to update these values with correct ones. Worth looking why with the change tests are not fully broken. We may need to test Span.service / Span.resource with fixed values (so getTag('service.name') instead of getTag(DDTags.SERVICE_NAME)) otherwise we're going to break things without knowing that.
If it's the case, would be great to add these changes to this PR.
There was a problem hiding this comment.
so you'd rather leave the (corrected) literal values in the tests instead of the field references?
same for resource-name
tylerbenson
force-pushed
the
tyler/consistency
branch
from
de740d7
to
aac9457
Compare
No description provided.