Fix API annotations discardable instrumentation

[eyalkoren]

What does this PR do?

Fixes an issue that was introduced through #2632 by the addition of the discardable attributes to the @CaptureSpan and @Traced annotations.

The application error looks as follows:

Caused by: java.lang.VerifyError: Bad type on operand stack
Exception Details:
...  
Reason:
    Type null (current frame, stack[5]) is not assignable to integer

The annotations instrumentation is tolerant to addition of annotation attributes, assigning such with null. The issue in this case is that the new attributes are of boolean type, which is not assignable from null.

Checklist

• This is a bugfix
  • I have updated the unit tests and Javadoc
  • I have updated the API documentation
  • I have updated CHANGELOG.asciidoc
  • I have fixated the relevant integration tests on public API version 1.30.0 and verified that they fail without this fix

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-06-29T09:16:40.019+0000

• Duration: 54 min 47 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3052
Skipped	36
Total	3088

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark tests.

• run jdk compatibility tests : Run the JDK Compatibility tests.

• run integration tests : Run the Agent Integration tests.

• run end-to-end tests : Run the APM-ITs.

• run windows tests : Build & tests on windows.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[felixbarny]

This would be a breaking change.
Maybe there's a way to use a non-constant stack manipulation here:

apm-agent-java/apm-agent-core/src/main/java/co/elastic/apm/agent/bci/bytebuddy/AnnotationValueOffsetMappingFactory.java

Line 50 in ee8c07b

return Target.ForStackManipulation.of(getAnnotationValue(instrumentedMethod, annotation.load()));

And use the wrapper type Boolean as a method argument here:

apm-agent-java/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/TracedInstrumentation.java

Line 75 in ee8c07b

@AnnotationValueOffsetMappingFactory.AnnotationValueExtractor(annotationClassName = "co.elastic.apm.api.Traced", method = "discardable") boolean discardable) {

[eyalkoren]

This would be a breaking change.

It was added in 1.32.0 for a specific user request, I think we can get away with that. Otherwise, it is currently a breaking change...

Maybe there's a way to use a non-constant stack manipulation

Hmm, interesting. I am not familiar with that enough. Are you proposing to work this area so that the advice is supplied with a different default, other than null? I'll look into it. The problem is that I cannot use a generic default to any boolean attribute, because the default in this case needs to be true, so I will have to do something specific for this attribute

[eyalkoren]

@felixbarny it worked! Thanks a lot ❤️

And use the wrapper type Boolean as a method argument here

This causes a VerifyError, probably because of what Byte Buddy makes of it at runtime, that mismatches the co.elastic.apm.api.Traced#discardable() signature. However, using the primitive boolean here works.