Name service based on servlet context #320
Conversation
tylerbenson
added
comp: core
tylerbenson
force-pushed
the
tyler/servlet-context-naming
branch
from
ca26224
to
26c794c
Compare
This only applies if a service name hasn’t been set or is empty. This is particularly useful for environments that deploy multiple war files to the same app server.
tylerbenson
force-pushed
the
tyler/servlet-context-naming
branch
from
26c794c
to
46878d2
Compare
tylerbenson
force-pushed
the
tyler/servlet-context-naming
branch
from
d80c40f
to
dc814ae
Compare
|
@realark I discovered that a lot of our tags weren't having the expected effect because they were set on the span builder, which didn't trigger any decorators. Now that decorators apply to the builder, a lot of things changed. Please review the changes and make sure the tests align with your expectations of how the instrumentation should work. We might need to change some instrumentation... |
There was a problem hiding this comment.
Nice catch on the span builder decorator issue! I don't have a strong opinion about what the tag values should be specificially, but consistency is good.
Let's get @palazzem's approval on the servlet service-naming then I'm good to merge.
There was a problem hiding this comment.
Using the context as a service name default is fine. IT ensures if you deploy multiple applications in the same (i.e.) Tomcat, you automatically have the split across all your apps.
I'm mostly worried about the change to the way we set tags. If you can address my question then I can see if it's ready for approval or not. Flagging as a request changes only to avoid a merge until it's clear to me the impact it has.
| @@ -95,18 +95,19 @@ class TomcatServlet3Test extends AgentTestRunner { | |||
| assertTraces(writer, 1) { | |||
| trace(0, 1) { | |||
| span(0) { | |||
| serviceName "unnamed-java-app" | |||
| serviceName "my-context" | |||
| @@ -49,10 +49,11 @@ class SpringBootBasedTest extends AgentTestRunner { | |||
| span.context().tags["span.kind"] == "server" | |||
| span.context().tags["span.type"] == "web" | |||
| span.context().tags["component"] == "java-web-servlet" | |||
| span.context().tags["servlet.context"] == "" | |||
There was a problem hiding this comment.
Should we really send this tag even if it's empty?
There was a problem hiding this comment.
Normally I’d say no, but in this case I think it might be useful to distinguish from something that may not have been set.
| @@ -369,15 +368,7 @@ public DDSpanBuilder withTag(final String tag, final Number number) { | |||
|
|
|||
| @Override | |||
| public DDSpanBuilder withTag(final String tag, final String string) { | |||
| if (tag.equals(DDTags.SERVICE_NAME)) { | |||
There was a problem hiding this comment.
This isn't applied anymore? or is it applied somewhere else? the way we set special Datadog tags is to ensure a change in internal attributes such as the spanType. Also, do we have tests for this functionality?
There was a problem hiding this comment.
Yes. All of these are handled consistently via decorators now.
This only applies if a service name hasn’t been set or is empty.
This is particularly useful for environments that deploy multiple war files to the same app server.
This is a potentially breaking change if someone hasn't configured a service name.