Replace/extend resource dest. with service target.{name.type}#2578
Replace/extend resource dest. with service target.{name.type}#2578SylvainJuge merged 51 commits intoelastic:mainfrom
Conversation
eyalkoren
left a comment
There was a problem hiding this comment.
Nice!
Several comments, nothing major.
I didn't go over all plugins because I saw that you used MockReporter#checkDestinationService to see that you didn't miss, but worth changing this flag and related methods to something like co.elastic.apm.agent.MockReporter#checkServiceTarget.
I did see that in many plugin tests you either replaced resource test with name and type, you didn't, but unless I am missing something, we should test both. If not - ignore.
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/ServiceTarget.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/ServiceTarget.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/ServiceTarget.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/DroppedSpanStats.java
Outdated
Show resolved
Hide resolved
...-dubbo-plugin/src/test/java/co/elastic/apm/agent/dubbo/AbstractDubboInstrumentationTest.java
Outdated
Show resolved
Hide resolved
...gins/apm-httpclient-core/src/main/java/co/elastic/apm/agent/httpclient/HttpClientHelper.java
Show resolved
Hide resolved
...ore/src/test/java/co/elastic/apm/agent/httpclient/AbstractHttpClientInstrumentationTest.java
Outdated
Show resolved
Hide resolved
.../apm-httpclient-core/src/test/java/co/elastic/apm/agent/httpclient/HttpClientHelperTest.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java
Outdated
Show resolved
Hide resolved
eyalkoren
left a comment
There was a problem hiding this comment.
LGTM
I made a couple of suggestions for very small but very easy perf improvements.
In addition, there are some comments from the original review that were still not addressed, mainly if the null check in serializer is relevant and whether we want to keep todo in code rather than have a meta issue
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/ServiceTarget.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/DroppedSpanStats.java
Show resolved
Hide resolved
| [float] | ||
| [[api-span-set-destination-resource]] | ||
| ==== `Span setDestinationService(String resource)` added[1.25.0] | ||
| Provides a way to manually set the span's `destination.service.resource` field, which is used for the | ||
| construction of service maps and the identification of downstream services. | ||
| Any value set through this method will take precedence over the automatically inferred one. | ||
| Using `null` or empty resource string will result in the omission of this field from the span context. | ||
|
|
There was a problem hiding this comment.
We should release not at the same day as the stack to make sure this didn't break cross-repo links
eyalkoren
left a comment
There was a problem hiding this comment.
Meant to approve and let you decide what to do with not yet addressed comments, just used the wrong checkbox 🙂
|
The comments should be fixed, I've opened elastic/apm#646 to enhance spec on other types of backends. |
What does this PR do?
Implements the agent-side migration from sending
span.destination.service.resourcetospan.service.target.type+span.service.target.name.Impacted features:
span.destination.service.resourcespan.destination.service.resourceThe agent remains backwards compatible with existing APM servers:
Checklist