opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Traced annotation #304

Open jpkrohling opened 5 years ago

jpkrohling commented 5 years ago

I'd like to host a @Traced annotation in this repository. Being part of the official API would remove this annotation from specific "downstream" repositories, such as:

opentracing-contrib/java-cdi eclipse/microprofile-opentracing

It could also be used here: https://github.com/opentracing-contrib/java-spring-cloud/issues/98

cc @ledor473, @pavolloffay

yurishkuro commented 5 years ago

I'm sceptical about this. The annotation by itself does not do anything, it must be used with a specific framework to make sense, so what's wrong with having it defined by the framework? Eg Spring has Autowired, wile the whildfly has Injected(?), each only makes sense in the context of the framework.

My concern with adding it here is people would then start asking "why is my function not traced after I annotated it".

ledor473 commented 5 years ago

I was tempted to send a PR about that, but the thing that made me doubt was mainly the @Target on the class:

@Target({ ElementType.METHOD, ElementType.ANNOTATION_TYPE })

Different AOP or Agent might only support on a method while others on both method and type or only on the type. I don't think it will be easy to come up with a "one size fits all" annotation.

The annotation I proposed in https://github.com/opentracing-contrib/java-spring-cloud/issues/98 included a withActiveSpanOnly property which would be useful for our usage, but might not make sense for all implemention.

jpkrohling commented 5 years ago

@ledor473 said:

but the thing that made me doubt was mainly the @Target on the class:

I can't think of a case where a framework would allow this annotation to be only at class level (or only at method level).

The annotation I proposed in opentracing-contrib/java-spring-cloud#98 included a withActiveSpanOnly property which would be useful for our usage, but might not make sense for all implemention

IMO, this is the strongest argument against having this annotation here.

@yurishkuro said:

My concern with adding it here is people would then start asking "why is my function not traced after I annotated it".

That would be equivalent of people asking why there's no concrete tracer being provided by this API, wouldn't it?

wu-sheng commented 5 years ago

SkyWalking has @Trace solution since we are providing features through annotation. https://github.com/apache/incubator-skywalking/blob/master/docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md

Also Spring framework can support that, only some frameworks have Class Scan mechanism and AOP.

yurishkuro commented 5 years ago

That would be equivalent of people asking why there's no concrete tracer being provided by this API, wouldn't it?

No, because they may already be using a concrete tracer, yet it won't do anything for the annotation. What is the value of having the annotation here as completely non-functional thing vs. in a library (e.g. opentracing-spring) for specific framework where it actually works?

jpkrohling commented 5 years ago

What is the value of having the annotation here as completely non-functional thing vs. in a library (e.g. opentracing-spring) for specific framework where it actually works?

For a business application, it wouldn't matter which components are seeing this annotation to make tracing decisions. So, they don't need to import io.springframework.something.Traced , org.eclipse.microprofile.something.Traced and/or io.opentracing.contrib.cdi.something.Traced.

Note that I'm also not convinced myself that it's a good idea to have it here, mostly because different frameworks might need different properties. So, I'd suggest leaving this here open and collect feedback from actual users as well, like @ledor473.

ledor473 commented 5 years ago

So here's the context and from where that idea came:

When I was looking at instrumenting code via an agent I found this: https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-api/src/main/java/datadog/trace/api/Trace.java

DD provides a small (no dependency) artifact that can be added to pretty much anything without risking to bring a new version of a library and cause a classpath hell.

That would let the user use a common annotation, while picking its own AOP solution. I felt it would be interesting to have that inside OpenTracing, because if we add it inside the Spring library and I'd like to use it with something non-Spring related, it fells wrong.

yurishkuro commented 5 years ago

I would put this into another library then, which can also have a lot more stable API and not depend on opentracing itself

codefromthecrypt commented 5 years ago

I rarely see Trace annotations that have the same features. It is unlikely this will be used generically across frameworks without them actively buying in. Before they'd buy in they'd likely need to agree to the same features. Agree that this shouldn't be in the core lib.

wuyupengwoaini commented 5 years ago

I am a normal developer and I think @Traced annotation is a very useful function and the same to other developers using opentracing.But adding @Traced annotation to opentracing is not a good choice.Instead,adding @Traced annotation to java-spring-cloud is a good choice.Futhermore, adding new tags to the existing span is also necessary,beacuse it offers a way to bind the trace to the User-defined tags like mobile or orderId and so on.

As for adding a new annotation or adding new method to the @Traced, i am confused.