AddingSpanAttributes annotation #7787

sfriberg · 2023-02-10T17:37:04Z

Expand the capabilities of SpanAttribute annotations to allow adding method parameters to and existing span on not only when the method is annotated with WithSpan.

To add to an existing span use the AddingSpanAttributes annotation on the method and apply SpanAttribute annotations as you would do with WithSpan.

As an example the following annotated method would if auto instrumentation is enabled add two attributes on every execution to the current span that is in context. If there is no current span no new span will be created. Similar to setAttribute any existing attributes will be overwritten.

  @AddingSpanAttributes
  public String withSpanAttributes(
      @SpanAttribute String implicitName,
      @SpanAttribute("explicitName") String parameter,
      String noAttr) {

    return "hello!";
  }

Suggestion for implementation of the discussion we had in the OTel Java meeting on 2023-02-02 and 2023-03-02.

linux-foundation-easycla · 2023-02-10T17:37:09Z

The committers listed above are authorized under a signed CLA.

✅ login: sfriberg (1144ffc)

mateuszrzeszutek

Thanks @sfriberg !

...in/java/io/opentelemetry/instrumentation/api/annotation/support/AttributeBindingFactory.java

...in/java/io/opentelemetry/instrumentation/api/annotation/support/SpanAttributesExtractor.java

...ry/javaagent/instrumentation/instrumentationannotations/AnnotationInstrumentationModule.java

...try/javaagent/instrumentation/instrumentationannotations/WithCurrentSpanInstrumentation.java

...ntation-annotations-1.16/javaagent/src/test/groovy/WithCurrentSpanInstrumentationTest.groovy

breedx-splk

I like this idea a lot and think it could be useful. I wonder if @WithSpan(useParent=true, ...) might possibly be more useful and less clashy? In any case, I think a few behaviors need to be clarified. Thanks!

...elemetry/javaagent/instrumentation/instrumentationannotations/AnnotationInstrumentation.java

breedx-splk · 2023-02-13T22:03:31Z

...ntation-annotations-1.16/javaagent/src/test/groovy/WithCurrentSpanInstrumentationTest.groovy

+          name "external"
+          kind INTERNAL


Seeing external and INTERNAL on the same span can be slightly misleading. Maybe rename external to parent or root for clarity?

breedx-splk · 2023-02-13T22:07:09Z

.../javaagent/src/test/java/io/opentelemetry/test/annotation/SpanAttributesWithCurrentSpan.java

+  @WithSpan
+  @WithCurrentSpan
+  public String withSpanAttributes(


I think a reader would quickly question this use of seemingly conflicting annotations. One explicitly creates a new span, one explicitly does not and uses the parent context. You can't have it both ways, so which one wins? It's not easily understood.

Looking at the test, it seems like @WithCurrentSpan wins here, because there's only 1 span in the resulting trace. Can you confirm that that is intentional?

Yes, that is intentional. The WithCurrenSpan instrumentation explicitly filters out any method that happens for some reason to be annotated with both.

isAnnotatedWith( named("application.io.opentelemetry.instrumentation.annotations.WithCurrentSpan")) // Avoid repeat extraction if method is already annotation with WithSpan .and( not( isAnnotatedWith( named( "application.io.opentelemetry.instrumentation.annotations.WithSpan"))));

What will happen if in the scope of a single span, 2 different methods with @WithCurrentSpan and their particular @SpanAttribute are invoked in chain?

It will overwrite in the same way calls to span.setAttribute(k,v) would if the key was already used.

breedx-splk · 2023-02-13T22:08:40Z

...ntation-annotations-1.16/javaagent/src/test/groovy/WithCurrentSpanInstrumentationTest.groovy

+      }
+    }
+  }
+}


I feel like there's also a missing test or two here --

what's the behavior if/when @WithCurrentSpan is used when there is no parent context?

what's the behavior if/when the current span already has an attribute with the same name defined?

linux-foundation-easycla · 2023-02-13T22:30:49Z

The committers listed above are authorized under a signed CLA.

✅ login: sfriberg (1144ffc, 604b68d)

sfriberg · 2023-02-13T23:08:36Z

I like this idea a lot and think it could be useful. I wonder if @WithSpan(useParent=true, ...) might possibly be more useful and less clashy?

I think this could be an interesting path. Probably name the variable useCurrent instead since you won't create a new span so won't really be a parent.

sfriberg · 2023-02-15T16:47:49Z

My ByteBuddy knowledge isn't good enough to figure out how to write an element matcher that also looks at the value of an annotation field, or if it is even possible.

So far what I have seen is that you can access that in your Advice methods which would be a bit of a hassle since you don't want to do extra work in the instrumented code.

mateuszrzeszutek · 2023-02-16T13:41:03Z

My ByteBuddy knowledge isn't good enough to figure out how to write an element matcher that also looks at the value of an annotation field, or if it is even possible.

I think you could do sth like this:

    declaresAnnotation(
        annotationType(
            named(
                "application.io.opentelemetry.instrumentation.annotations.WithSpan"))
            .and(new AnnotationHasBooleanValue("useCurrent", true)));

where AnnotationHasBooleanValue is

  static final class AnnotationHasBooleanValue implements ElementMatcher<AnnotationDescription> {

    private final String fieldName;
    private final boolean fieldValue;

    AnnotationHasBooleanValue(String fieldName, boolean fieldValue) {
      this.fieldName = fieldName;
      this.fieldValue = fieldValue;
    }

    @Override
    public boolean matches(AnnotationDescription target) {
      AnnotationValue<?, ?> value = target.getValue(fieldName);
      return value.resolve(boolean.class) == fieldValue;
    }
  }

I haven't tested it though.

brunobat · 2023-02-22T10:56:22Z

...-annotations/src/main/java/io/opentelemetry/instrumentation/annotations/WithCurrentSpan.java

+ */
+@Target({ElementType.METHOD, ElementType.CONSTRUCTOR})
+@Retention(RetentionPolicy.RUNTIME)
+public @interface WithCurrentSpan {}


What if there is not current span? Will it create one?
Should we name it @SpanAttributes (plural) or similar, instead?

If now span exists nothing will happen. Working on an updated impl which will instead reuse WithSpan with a variable useCurrent to select if you want a new Span or reuse an existing one.

brunobat · 2023-02-22T10:59:16Z

.../javaagent/src/test/java/io/opentelemetry/test/annotation/SpanAttributesWithCurrentSpan.java

+  @WithSpan
+  @WithCurrentSpan
+  public String withSpanAttributes(


What will happen if in the scope of a single span, 2 different methods with @WithCurrentSpan and their particular @SpanAttribute are invoked in chain?

sfriberg · 2023-02-25T19:08:05Z

@mateuszrzeszutek Thanks for the pointers was able to get things working. Only thing was that you must use Boolean.class rather than boolean.class it seems like.

However as I tested things and started to write some Javadoc I realized that reusing WithSpan will probably cause more confusion that simplify things. If you set useCurrent all other variable basically are ignored since we will not change the span name or kind which you can still set. So I think having a separate annotation will make it clearer what the behaviour is.

Since the focus is really just span attributes I kind of like the name suggested by @brunobat, but WithCurrentSpan also keeps it close to the current naming rather than changing things completely.

I should be able to join this coming Thursday and perhaps we can make a quick decision there.

Agree on separate annotation
If agggrement what should the name be @WithCurrentSpan or @SpanAttributes

trask · 2023-03-02T17:13:59Z

.../javaagent/src/test/java/io/opentelemetry/test/annotation/SpanAttributesWithCurrentSpan.java

+
+public class SpanAttributesWithCurrentSpan {
+
+  @WithCurrentSpan


alternatives:

WithSpanAttributes ✅

UpdateCurrentSpan

mateuszrzeszutek

Thanks @sfriberg !

...ry/javaagent/instrumentation/instrumentationannotations/AnnotationInstrumentationModule.java

....0/javaagent/src/test/java/io/opentelemetry/test/annotation/WithSpanInstrumentationTest.java

sfriberg · 2023-03-22T20:11:36Z

Anything further for me todo, or ready to be approved and merged?

mateuszrzeszutek · 2023-03-23T08:22:50Z

IMO it's 👍 as it is.
@trask @laurit is there anything you want to add here? (This PR adds a new thing to a stable instrumentation-annotations, which is why I'd like other maintainers to take a look at it as well)

trask

thx @sfriberg!

laurit · 2023-04-04T09:35:19Z

@sfriberg could you rebase so that this pr could be merged

trask · 2023-04-13T14:22:09Z

@sfriberg sorry, I had second thoughts on the annotation name because "with" implies to me "in the scope of", or possibly immutable, and this is mutating something already in scope. we discussed in Java SIG meeting and came up with @AddingSpanAttributes. If you don't mind renaming once more 🙇‍♂️

tylerbenson · 2023-04-13T18:25:33Z

@sfriberg would you mind adding some code snippet examples in the PR description so it's easier to understand what this does and how it should be used?

Co-authored-by: Mateusz Rzeszutek <[email protected]>

…ent/src/test/java/io/opentelemetry/test/annotation/WithSpanInstrumentationTest.java Co-authored-by: Mateusz Rzeszutek <[email protected]>

…/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationannotations/AnnotationInstrumentationModule.java Co-authored-by: Mateusz Rzeszutek <[email protected]>

sfriberg · 2023-04-23T17:39:02Z

@trask No problem, think the name changes make sense. Should be all updated now.
@tylerbenson Expand the PR intro to include a brief example. Hope this explains it enough, otherwise just let me know.

...est/java/io/opentelemetry/instrumentation/annotations/AddingSpanAttributesUsageExamples.java

tylerbenson

@sfriberg thanks for improving the description. That makes things much clearer.

trask · 2023-04-24T15:58:55Z

@sfriberg thx 🎉 (and thx for your patience)

LoganDev99 · 2023-11-15T04:32:43Z

Does @SpanAttribute support fields of an object or just scalar fields?

For example, using a Customer object. Does that work out the box? Or is it possible to pick only some fields from Customer?

  @AddingSpanAttributes
  public String withSpanAttributes(
      @SpanAttribute Customer customer
) {

    return "hello!";
  }

mateuszrzeszutek · 2023-11-15T16:19:13Z

For example, using a Customer object. Does that work out the box?

AFAIR it'll just set the value to the result ofcustomer.toString(). We don't really support any nested object access, expression language etc. For more complex cases, you can always call

Span.current().setAttribute("something", customer.getSomething());

sfriberg requested a review from a team February 10, 2023 17:37

sfriberg force-pushed the spanattributes branch from 652c565 to 1144ffc Compare February 10, 2023 17:39

mateuszrzeszutek reviewed Feb 13, 2023

View reviewed changes

breedx-splk reviewed Feb 13, 2023

View reviewed changes

sfriberg force-pushed the spanattributes branch from 33e0dd3 to 604b68d Compare February 13, 2023 22:32

brunobat reviewed Feb 22, 2023

View reviewed changes

trask reviewed Mar 2, 2023

View reviewed changes

sfriberg changed the title ~~WithCurrentSpan annotation~~ WithSpanAttributes annotation Mar 3, 2023

mateuszrzeszutek approved these changes Mar 7, 2023

View reviewed changes

trask approved these changes Apr 2, 2023

View reviewed changes

sfriberg and others added 9 commits April 23, 2023 10:21

WithCurrentSpan annotation

1a9f3ac

PR comments

1285315

More PR comments

068e303

More PR comments

e9d24dc

reduce order value

4392bb5

Co-authored-by: Mateusz Rzeszutek <[email protected]>

Rename to WithSpanAttributes

34a7073

spotless

a7a79f9

Update instrumentation/opentelemetry-extension-annotations-1.0/javaag…

177a5c3

…ent/src/test/java/io/opentelemetry/test/annotation/WithSpanInstrumentationTest.java Co-authored-by: Mateusz Rzeszutek <[email protected]>

Update instrumentation/opentelemetry-extension-annotations-1.0/javaag…

f7bdf78

…ent/src/test/java/io/opentelemetry/test/annotation/WithSpanInstrumentationTest.java Co-authored-by: Mateusz Rzeszutek <[email protected]>

Update instrumentation/opentelemetry-instrumentation-annotations-1.16…

f7baa03

…/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationannotations/AnnotationInstrumentationModule.java Co-authored-by: Mateusz Rzeszutek <[email protected]>

sfriberg force-pushed the spanattributes branch from 19bbf3c to f7baa03 Compare April 23, 2023 15:28

sfriberg added 2 commits April 23, 2023 12:29

Rename annotation

5d1e3f1

Rename annotation

4b37a22

sfriberg changed the title ~~WithSpanAttributes annotation~~ AddingSpanAttributes annotation Apr 23, 2023

mateuszrzeszutek approved these changes Apr 24, 2023

View reviewed changes

...est/java/io/opentelemetry/instrumentation/annotations/AddingSpanAttributesUsageExamples.java Show resolved Hide resolved

tylerbenson approved these changes Apr 24, 2023

View reviewed changes

trask merged commit d1b7356 into open-telemetry:main Apr 24, 2023

trask mentioned this pull request Jul 24, 2023

Annotations to inject and extract data with auto-instrumentation #9128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AddingSpanAttributes annotation #7787

AddingSpanAttributes annotation #7787

sfriberg commented Feb 10, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 10, 2023 •

edited

Loading

mateuszrzeszutek left a comment

breedx-splk left a comment

breedx-splk Feb 13, 2023

breedx-splk Feb 13, 2023

sfriberg Feb 13, 2023

sfriberg Feb 13, 2023

brunobat Feb 22, 2023

sfriberg Feb 25, 2023 •

edited

Loading

breedx-splk Feb 13, 2023

linux-foundation-easycla bot commented Feb 13, 2023 •

edited

Loading

sfriberg commented Feb 13, 2023

sfriberg commented Feb 15, 2023

mateuszrzeszutek commented Feb 16, 2023 •

edited

Loading

brunobat Feb 22, 2023

sfriberg Feb 25, 2023

brunobat Feb 22, 2023

sfriberg commented Feb 25, 2023

trask Mar 2, 2023 •

edited

Loading

mateuszrzeszutek left a comment

sfriberg commented Mar 22, 2023

mateuszrzeszutek commented Mar 23, 2023

trask left a comment

laurit commented Apr 4, 2023

trask commented Apr 13, 2023

tylerbenson commented Apr 13, 2023

sfriberg commented Apr 23, 2023

tylerbenson left a comment

trask commented Apr 24, 2023

LoganDev99 commented Nov 15, 2023

mateuszrzeszutek commented Nov 15, 2023


		public class SpanAttributesWithCurrentSpan {

		@WithCurrentSpan

AddingSpanAttributes annotation #7787

AddingSpanAttributes annotation #7787

Conversation

sfriberg commented Feb 10, 2023 • edited Loading

linux-foundation-easycla bot commented Feb 10, 2023 • edited Loading

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

breedx-splk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfriberg Feb 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Feb 13, 2023 • edited Loading

sfriberg commented Feb 13, 2023

sfriberg commented Feb 15, 2023

mateuszrzeszutek commented Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfriberg commented Feb 25, 2023

trask Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

sfriberg commented Mar 22, 2023

mateuszrzeszutek commented Mar 23, 2023

trask left a comment

Choose a reason for hiding this comment

laurit commented Apr 4, 2023

trask commented Apr 13, 2023

tylerbenson commented Apr 13, 2023

sfriberg commented Apr 23, 2023

tylerbenson left a comment

Choose a reason for hiding this comment

trask commented Apr 24, 2023

LoganDev99 commented Nov 15, 2023

mateuszrzeszutek commented Nov 15, 2023

sfriberg commented Feb 10, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 10, 2023 •

edited

Loading

sfriberg Feb 25, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 13, 2023 •

edited

Loading

mateuszrzeszutek commented Feb 16, 2023 •

edited

Loading

trask Mar 2, 2023 •

edited

Loading