-
Notifications
You must be signed in to change notification settings - Fork 828
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
Add typed spans #502
Add typed spans #502
Conversation
Thank you @thisthat for this! :) I will take a closer look and play with this during weekend. |
I ran some benchmark for both variants using the SpanPipelineBenchmark as a template. On my laptop, they all perform the same. |
@thisthat How will these spans work with |
Will that mean that all "type" information will be lost as soon as typed span is put into context? And we cannot "convert" regular span into typed span. Will play more with this tomorrow... |
Some notes and assorted thoughts. I don't understand the My biggest question/concern atm is why these spans does not form actual hierarchy: Is it expected that not all attributes have setters? E.g. I like the idea of checking for mandatory attributes :) But have you thought about some way to make it easier for clients to understand, what attributes are mandatory? Before they try to create a span and see warnings in logs. Have you thought how these new classes will play with Other nitty-gritty comments/suggestions can be addressed later, I think. |
@iNikem thank you for the valuable feedback!
BasicXXX classes are used to generify over Span and Builder types, whereas the XXXSpan use concrete types for these. I could not find another way to implement this. If you have a better idea, I am open to suggestions :)
Typed Spans do not form a hierarchy but encapsulate the logic of a Span to facilitate a given Semantic Convention. Implementing the
Yes! Since the HTTP Server Semantic Convention requests only
Yes. A class-level Javadoc should suffice for this purpose. However, in a utopian world, all attributes should be set ;)
Yes. Typed Spans and SemanticAttributes are next to each other. Using typed spans, we can achieve the same level of type safety for attributes. I think using SemanticAttributes inside Typed Spans is not really useful since it simply encapsulates the |
Sorry, still don't understand. If |
But they are required by https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/span-general.md, aren't they? |
Isn't the whole idea of semantic spans: to have "some extra methods"? :) Implementing |
I would argue for the contrary. Typed Spans are used to reduce the API surface to only those methods that contribute to a specific semantic convention.
Currently, the I don't see the benefit of storing directly a typed span into the context but, I am absolutely not an expert on this topic. Could you please provide an example where this is useful to help me understand better? |
We want to generalize the setters' return type for both, Span and Builder. Since we have the possibility of "extends", we need a hierarchy of classes that has no concrete type to allow at any point in the type chain to return the general type. I was not able to do this in a single class. But maybe I cannot see the easier solution! |
There is no hard requirement in this document. It says only it 'may be used for any network related operation'. Or am I reading wrongly the specification? |
So here is the main difference in our opinions :) For me, HttpServerSpan is-a Span, it just provides higher-level, more convenient API for the specific kind of a span. It makes certain things simpler, but does not make already existing things harder. Requiring one extra call to By the same reasoning, one can expect that I am probably going too esoteric here, but your proposal essentially means that
Main benefit for me is the ability to take a span from the context and be able to downcast it to a specific semantic type. E.g. take a look at this class. I wait for semantic spans to return |
@iNikem your last comment was really informative. I now see your point of view. I have added a discussion point into the Java Instrumentation agenda so we can have a discussion this Thursday :) |
686df44
to
572534d
Compare
That looks nice! :) Please allow me a couple of days to play with this and to migrate some existing instrumentations to it. Then I can provide meaningful feedback. |
Sure @iNikem! thanks for doing that :) |
I like this much more than the first version :) Semantic convention interfaces are very descriptive. Much more than original spans with all their implementations 👍 Some small remarks:
I will definitely need more time with them, but I think they are almost ready to be merged and be battle-tested :) |
As a high level feedback, is my understanding correct that the main result of this change is to translate the relationships defined semantic conventions from the spec repo into code? That seems like a nice idea :) I'm skimming through open-telemetry/opentelemetry-java#778 and there doesn't seem to have been a conclusion, but +several to moving this into I also noticed that the linked issue mentions typed tracers - is that similar to Brave's parser approach? I think these parsers are similar to the decorators in this repo, and I find it to be a nice pattern since wrapping library objects with an adapter into an instrumentation object generally works pretty well. I added and I'm guessing long term, we would expect |
Yeah, the main goal is to codify semantic conventions. Currently chosen path is to develop and stabilise them in instrumentation repo and then, after they are stable, move them (or part of) into main java repo. We already have 2 examples of "typed/semantic" tracers: |
The typed tracers seem similar to the decorators at first glance, haven't dug enough to find the difference. But it does seem to have a similar concept of exposing getters for semantic conventions Isn't the goal for this to be the complete set of semantic conventions, so would have a very similar API to the typed spans, though as getters instead of setters? |
You referenced method is protected, not public. It is meant to be implemented by subclasses, which know how to get peer port from the specific library. Decorator and tracers are indeed very similar. Main difference, et least for me, is design goal. Tracers are meant to have as small public API as possible. Ideally just 2 methods: |
I think we're talking about the same thing :) By API, I just meant that the instrumentation will have to implement the getters, but indeed they should only be calling those two methods. Brave works similarly, may be worth a look if there are any pointers. |
I am not familiar with this repo, I trust your opinion for the package :)
Yes, exactly.
That's an option that I did not consider yet. The design decision was to provide a straightforward API to set semantic attributes. However, handling special attributes with more fine graned methods sounds a valid approach!
Thanks for noticing it :) I will add the method! |
Yeah, |
@thisthat have you had a chance to address latest comments? |
@iNikem sorry to let you wait. Unfortunately, I am quite busy rn. I will have some free cycles next week so expect something for that timeframe 😉 |
@thisthat does your time estimate still hold? :) Can we help you somehow? |
@iNikem I started addressing your feedback and adding all remaining semantic conventions ;) Expect something for tomorrow/Wednesday! |
...t-bootstrap/src/main/java/io/opentelemetry/auto/typedspan/DbCassandraSemanticConvention.java
Outdated
Show resolved
Hide resolved
public DbCassandraSemanticConvention setDbUser(String dbUser); | ||
|
||
/** | ||
* Sets a value for db.mssql.instance_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is Cassandra convention, it cannot have db.mssql*
or db.jdbc.driver_classname
agent-bootstrap/src/main/java/io/opentelemetry/auto/typedspan/DbCassandraSpan.java
Show resolved
Hide resolved
agent-bootstrap/src/main/java/io/opentelemetry/auto/typedspan/DbCassandraSpan.java
Outdated
Show resolved
Hide resolved
agent-bootstrap/src/main/java/io/opentelemetry/auto/typedspan/DbCassandraSpan.java
Show resolved
Hide resolved
agent-bootstrap/src/main/java/io/opentelemetry/auto/typedspan/HttpServerSemanticConvention.java
Show resolved
Hide resolved
agent-bootstrap/src/main/java/io/opentelemetry/auto/typedspan/HttpServerSpan.java
Show resolved
Hide resolved
Thank you, @thisthat :) This is going to rock. Semantic convention codified in Spans will make both auto and manual instrumentation much pleasant. And their generation will make syncing with convention changes much easier. I am sure, when we start actually using them a lot more suggestions will come, but this is very solid starting point. How far are you from making generator itself public? And how do you scrape conventions now? Even if your generator is not yet ready, I suggest bringing this PR up to date with the current Java API and merging it. We can start using it and discovering what modifications we need to introduce. We can fix them manually and feed this info back to generator. @thisthat @trask @anuraaga what do you think? |
I am pretty close to release it. I am currently polishing the few remaining details. Expect something in one of the following SIG Spec meeting 😉
To what API are you referring to? |
otel-java API. This PR currently does not compile against 0.6.0 version. |
88f1711
to
f117a8b
Compare
@thisthat I still don't see that builders extend |
If we make the Since typed spans exports all methods for a given semantic convention, I don't think using |
Ok, makes sense. |
If there are no more comments, I will mark it as ready for review! :) |
@thisthat is there any way to split this up into smaller PRs? Given the current size, this PR is very difficult to review without getting lost. |
Sure, I can split this draft into smaller PRs. One for each semantic convention group! |
Mind that the current code is automatically generated from a template, see https://gist.github.com/thisthat/7e34742f4a7f1b5df57118f859a19c3b. So the changes are (mainly) only about class, method, and parameter names. |
hey @thisthat, do we need to check in the generated code? |
@trask currently, the idea is to have this code in the repo. This way, people can start using it and see problems/benefits and adapt the code if we find something that does not work or could be improved :) |
@thisthat If you agreed on submitting smaller PRs for independent pieces, then I propose to close PR |
Should I wait for open-telemetry/opentelemetry-specification#653 to be resolved before proposing typed spans in smaller PRs? |
I don't think it makes sense. Spec issues may take a lot of time. And as you generate code, it will be easy to remove required checks later. So please proceed with PRs :) |
This draft proposes two API variants for typed spans addressing the issue open-telemetry/opentelemetry-java#778. It uses the HTTP and HTTP Server semantic conventions as an exemplar for how the APIs look like.
The first variant is in the
flat
package, whereas the second is in thehierarchy
package.Both variants provide facility methods to set relevant attributes of a given Semantic Convention. Typed spans delegate to the consumer of the API the knowledge of the source values for these attributes.
Furthermore, warnings are logged if important attributes of the Semantic Convention were not provided before terminating the span.