-
Notifications
You must be signed in to change notification settings - Fork 324
Reuse SpanKind Entry in ClientDecorator #10503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
9482232
8720c24
fbec328
2864bac
43bbea1
5fadff5
3c0997f
b192679
cffac87
8c51f7f
17b1255
e18b404
5698bff
9d43d47
26cab22
f6ceef1
501ade2
40847ac
d96ebfb
9f64448
b7327b5
6fa193a
c97cdee
531f63b
2b89bfc
db10ed5
37d1009
6f2160f
4d0be98
aa1f66b
2959010
017f930
605998f
a01fc69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,24 @@ | ||
| package datadog.trace.bootstrap.instrumentation.decorator; | ||
|
|
||
| import datadog.trace.api.TagMap; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import datadog.trace.bootstrap.instrumentation.api.Tags; | ||
|
|
||
| public abstract class ClientDecorator extends BaseDecorator { | ||
| // Deliberately not volatile, reading a stale null and creating an extra Entry is safe | ||
| private TagMap.Entry cachedSpanKindEntry = null; | ||
|
|
||
| protected abstract String service(); | ||
|
|
||
| /** Caches span kind entry to reduce allocation */ | ||
| private final TagMap.Entry spanKindEntry() { | ||
| TagMap.Entry kindEntry = cachedSpanKindEntry; | ||
| if (kindEntry == null) { | ||
| cachedSpanKindEntry = kindEntry = TagMap.Entry.create(Tags.SPAN_KIND, spanKind()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a side note, this could have been done in the constructor if spanKind had been passed as a parameter instead of being defined as an overridable method.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I would very much like to refactor the decorators to make such things possible. But I do think we really need to revisit the decorators, I think are adding a lot of unnecessary overhead.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. I just realized that the current structure conceals invisible contracts, such as the requirement that spanKind not change. |
||
| } | ||
| return kindEntry; | ||
| } | ||
|
|
||
| protected String spanKind() { | ||
| return Tags.SPAN_KIND_CLIENT; | ||
| } | ||
|
|
@@ -16,7 +28,7 @@ public AgentSpan afterStart(final AgentSpan span) { | |
| if (service() != null) { | ||
| span.setServiceName(service()); | ||
| } | ||
| span.setTag(Tags.SPAN_KIND, spanKind()); | ||
| span.setTag(spanKindEntry()); | ||
|
|
||
| // Generate metrics for all client spans. | ||
| span.setMeasured(true); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,7 +104,7 @@ public void createEmptyCharSequence() { | |
| } | ||
|
|
||
| @Test | ||
| public void objectEntry() { | ||
| public void newObjectEntry() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is clean-up of tests that I missed on the original PR. |
||
| test( | ||
| () -> TagMap.Entry.newObjectEntry("foo", "bar"), | ||
| TagMap.Entry.OBJECT, | ||
|
|
@@ -119,7 +119,7 @@ public void objectEntry() { | |
| } | ||
|
|
||
| @Test | ||
| @DisplayName("anyEntry: Object") | ||
| @DisplayName("newAnyEntry: Object") | ||
| public void anyEntryObject() { | ||
| test( | ||
| () -> TagMap.Entry.newAnyEntry("foo", "bar"), | ||
|
|
@@ -155,7 +155,7 @@ public void createBoolean(boolean value) { | |
| @ValueSource(booleans = {false, true}) | ||
| public void newBooleanEntry(boolean value) { | ||
| test( | ||
| () -> TagMap.Entry.newBooleanEntry("foo", value), | ||
| () -> TagMap.Entry.create("foo", value), | ||
| TagMap.Entry.BOOLEAN, | ||
| (entry) -> | ||
| multiCheck( | ||
|
|
@@ -253,7 +253,7 @@ public void newIntEntryBoxed(int value) { | |
| @ParameterizedTest | ||
| @DisplayName("newIntEntry: Short") | ||
| @ValueSource(shorts = {Short.MIN_VALUE, -256, -128, -1, 0, 1, 128, 256, Short.MAX_VALUE}) | ||
| public void intEntryBoxedShort(short value) { | ||
| public void newIntEntryBoxedShort(short value) { | ||
| test( | ||
| () -> TagMap.Entry.newIntEntry("foo", Short.valueOf(value)), | ||
| TagMap.Entry.INT, | ||
|
|
@@ -269,7 +269,7 @@ public void intEntryBoxedShort(short value) { | |
| @ParameterizedTest | ||
| @DisplayName("newIntEntry: Byte") | ||
| @ValueSource(bytes = {Byte.MIN_VALUE, -32, -1, 0, 1, 32, Byte.MAX_VALUE}) | ||
| public void intEntryBoxedByte(byte value) { | ||
| public void newIntEntryBoxedByte(byte value) { | ||
| test( | ||
| () -> TagMap.Entry.newIntEntry("foo", Byte.valueOf(value)), | ||
| TagMap.Entry.INT, | ||
|
|
@@ -500,7 +500,7 @@ public void createDouble(double value) { | |
| @DisplayName("newDoubleEntry: double") | ||
| @ValueSource( | ||
| doubles = {Double.MIN_VALUE, Float.MIN_VALUE, -1D, 0D, 1D, Math.E, Math.PI, Double.MAX_VALUE}) | ||
| public void doubleEntry(double value) { | ||
| public void newDoubleEntry(double value) { | ||
| test( | ||
| () -> TagMap.Entry.newDoubleEntry("foo", value), | ||
| TagMap.Entry.DOUBLE, | ||
|
|
@@ -532,7 +532,7 @@ public void newDoubleEntryBoxed(double value) { | |
| @DisplayName("newAnyEntry: Double") | ||
| @ValueSource( | ||
| doubles = {Double.MIN_VALUE, Float.MIN_VALUE, -1D, 0D, 1D, Math.E, Math.PI, Double.MAX_VALUE}) | ||
| public void anyEntry_double(double value) { | ||
| public void newAnyEntryDouble(double value) { | ||
| test( | ||
| () -> TagMap.Entry.newAnyEntry("foo", Double.valueOf(value)), | ||
| TagMap.Entry.ANY, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.