Add annotations to create transactions and spans #281
Conversation
eyalkoren
left a comment
eyalkoren
left a comment
There was a problem hiding this comment.
The unit test demonstrates how great this API is!
Several comments/clarifications and a suggestion to add a class-level annotation just to make type matching more efficient on the expense of slightly more complicated API.
| @Override | ||
| public ElementMatcher<? super TypeDescription> getTypeMatcher() { | ||
| return isInAnyPackage(config.getApplicationPackages(), ElementMatchers.<NamedElement>none()) | ||
| .<TypeDescription>and(not(nameContains("$MockitoMock$"))) |
There was a problem hiding this comment.
I had some transformation errors but I don't seem to get them now, deleting for now.
| public ElementMatcher<? super TypeDescription> getTypeMatcher() { | ||
| return isInAnyPackage(config.getApplicationPackages(), ElementMatchers.<NamedElement>none()) | ||
| .<TypeDescription>and(not(nameContains("$MockitoMock$"))) | ||
| .and(declaresMethod(getMethodMatcher())); |
There was a problem hiding this comment.
I don't think this adds performance advantage, as it means you invoke the method matcher on all methods of all examined classes anyway.
For the sake of performance, we can add a class-level annotation, something like @Instrumented or @Captured to help us with efficient type matching
There was a problem hiding this comment.
This is not for performance but to avoid type matches being logged even though no methods get instrumented.
There was a problem hiding this comment.
We are only scanning the application packages, so I don't know if this is needed. Depends on the size of the code base if that's worth it so evaluating the performance with our toy applications does probably not tell the full story...
There was a problem hiding this comment.
We are only scanning the application packages
Right, missed the none default. I assumed (and still think) any is a better default, so that we don't force users to configure packages.
Anyway, if they are not going to get anything recorded without that configuration, the API documentation should state that in a very visible way
| @Advice.Local("span") Span span) { | ||
| if (tracer != null) { | ||
| final AbstractSpan<?> parent = tracer.activeSpan(); | ||
| if (parent != null) { |
There was a problem hiding this comment.
Also check parent.isSampled()
There was a problem hiding this comment.
Even if the parent is not sampled, we should create a span, for example for log correlation.
There was a problem hiding this comment.
It's not how we treat it in our instrumentation. Is that because it is an API?
| span = parent.createSpan() | ||
| .withName(transactionName.isEmpty() ? signature : transactionName) | ||
| .withType(type) | ||
| .activate(); |
There was a problem hiding this comment.
should we check whether span.isSampled() before activating it?
There was a problem hiding this comment.
We need to activate non sampled spans to aid log correlation.
...ins/apm-api-plugin/src/main/java/co/elastic/apm/plugin/api/ElasticApmApiInstrumentation.java
Show resolved
Hide resolved
apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java
Show resolved
Hide resolved
apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationApiTest.java
Show resolved
Hide resolved
| * The name of the {@link Span}. | ||
| * Defaults to the {@code ClassName#methodName} | ||
| */ | ||
| String value() default ""; |
There was a problem hiding this comment.
Should be called name or spanName
There was a problem hiding this comment.
The method value has special semantics in annotations. It is the default property when declaring an annotation. That means we can do @CaptureSpan("name") instead of @CaptureSpan(name = "name"). I think the most frequent use case would be to only add a span name but to not care about the span type.
| * The name of the {@link Transaction}. | ||
| * Defaults to the {@code ClassName#methodName} | ||
| */ | ||
| String value() default ""; |
There was a problem hiding this comment.
Should be called name or transactionName
...ent-core/src/main/java/co/elastic/apm/bci/bytebuddy/AnnotationValueOffsetMappingFactory.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #281 +/- ##
============================================
- Coverage 73.11% 72.68% -0.44%
- Complexity 1165 1179 +14
============================================
Files 126 130 +4
Lines 4385 4440 +55
Branches 440 447 +7
============================================
+ Hits 3206 3227 +21
- Misses 970 1007 +37
+ Partials 209 206 -3
Continue to review full report at Codecov.
|
3 participants
closes #212