Addresses #1043: add a serviceName attribute to the @Trace annotation

[msm]

Hi there,

This PR is to address/resolve #1043. I will be the first to apologize as I have not been able to spend the time yet to setup all of the different jvms, environment, etc., in order to build and run the tests for this.

I am hoping that given this is a relatively minimal and straightforward change (essentially mirroring what is already being done for resourceName) this at least increases the chances of #1043 getting fixed :)

One question I have here is whether it is kosher to use span.setServiceName(..) versus using the pattern I've seen elsewhere of activeSpan().setTag(DDTags.SERVICE_NAME, ..);. I didn't look too closely at the pros/cons of using the direct method vs setting the tag value, but I'm happy to change this PR to set the tag value instead if that is preferred.

Thanks in advance for your consideration!

[richardstartin]

This needs to be "SayTracedHello.sayHelloWithServiceName" because the resource name is derived from the method name unless specified, this would make the tests pass.

[richardstartin]

Hi @msm this looks good, and a useful feature so thanks for the contribution!

The muzzle failure is complaining that the instrumentation isn't backward compatible with previously released versions of 'com.datadoghq:dd-trace-api', we can get around this but it's a little complicated and not necessarily a great contributor experience since there are some circular dependencies to break. We would probably need to first release dd-trace-api with a service name attribute, and then implement a new trace annotation instrumentation as a sibling to the current trace-annotation module which only activates when the @Trace class has the serviceName attribute, with the current instrumentation only activating when the serviceName attribute is absent from @Trace.

So, if this is something you need, we can do this for you, and hopefully use some of the commits from your branch, but it wouldn't be safe to merge this branch.

[richardstartin]

There may be other ways to implement this - is there a reason you want certain service names on certain annotated spans? Or would it be enough to override the service name for all spans in the trace via -Ddd.service="my service name"?

[msm]

Hi @richardstartin Thanks for the quick (and positive) response!

Let me start by saying, I'm totally fine with you throwing this entire branch away if its not useful - won't hurt my feelings!

One other thought, in case it would be more straightforward, would be adding a separate @TraceService(String name) annotation that could optionally decorate a method alongside the existing @Trace(..) annotation. I think from the perspective of the end-user though, this wouldn't be ideal.

My other initial thought would be to check for the presence of the serviceName attribute on the annotation interface using reflection, rather than blindly assuming it is present. I'm guessing the performance hit from reflection would make that strategy unacceptable?

[msm]

There may be other ways to implement this - is there a reason you want certain service names on certain annotated spans? Or would it be enough to override the service name for all spans in the trace via -Ddd.service="my service name"?

My use case is similar to the issue the OP was discussing in #1043 - I have a bunch of calls I am making (using a legacy WSDL-based webservice & JAX-WS) that take up quite a bit of wall clock time. I'm trying to pull that out on the span flame graphs from being attribute to my application's service, into a separate "virtual service" name, like java-aws-sdk or twilio-sdk appear to be separated automatically.

In this case, having a true integration to intercept and instrument JAX-WS is probably a cleaner solution overall, however it looks to be quite a bit more work to do that.

[richardstartin]

One other thought, in case it would be more straightforward, would be adding a separate @TraceService(String name) annotation that could optionally decorate a method alongside the existing @trace(..) annotation. I think from the perspective of the end-user though, this wouldn't be ideal.

In the short term, you're right, but in the long term this increases the size of our API and makes it harder to use.

My other initial thought would be to check for the presence of the serviceName attribute on the annotation interface using reflection, rather than blindly assuming it is present. I'm guessing the performance hit from reflection would make that strategy unacceptable?

It's a good point but we try not to do things like that because we like to only load instrumentations if we know they are safe to load given runtime information about the application's classpath. There are places where reflection is unavoidable and we do use it, but muzzle (which aims to ensure safety) can't follow reflective references. We've also been discussing reworking this particular instrumentation to avoid doing any reflection at all to make it more efficient (so generating the advice from the values found in the annotation, with no need to look up the Method or call Method.getAnnotation(Trace.class)), and this would be a step in the opposite direction.

[richardstartin]

In this case, having a true integration to intercept and instrument JAX-WS is probably a cleaner solution overall, however it looks to be quite a bit more work to do that.

This is something we may implement soon since there is some demand for tracing SOAP.

[msm]

This is something we may implement soon since there is some demand for tracing SOAP.

That sounds useful! There are a few other scenarios where JAX-WS instrumentation support isn't an option however, where I think it would be useful to "pull out" and wrap calls to third-party or otherwise external services.

I didn't realize how what looked like a minor change that brings things in line with other datadog SDKs would entail a more involved process to release in a backwards compatible manner, although I understand that now. Since workarounds do exist for my objective, I don't think this specific request of adding serviceName to the @Trace annotation is critical, although it would definitely be a nice-to-have the next time you are in this area and have to make a breaking change that requires backwards compatibility workarounds!

[richardstartin]

Hi @msm we merged support for JAX-WS tracing in #2473 and it will be available in the next release (should be released within a week) - do you still want me to go ahead and finish this off to maintain backward compatibility?

[msm]

Hii @richardstartin thats great news about #2473! Very cool!

I think my feelings on this are pretty similar to how they were in my last comment - I do not feel you should spend a lot of time on implementing this, but if there is a time where you have to make additional backwards incompatible changes in the future and the marginal cost of adding this too is low, I think it would be a net win for the platform.

[bm1549]

@msm first off I'd like to thank you for your contribution

To support many of our newer features, Datadog is moving away from manipulating the service tag for traces coming from a service, which includes the proposed changes in this PR. As such, I'm going to close it out

If this comes up again, please open up a new issue for further discussion