Skip to content
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

Replace/extend resource dest. with service target.{name.type} #2578

Merged
merged 51 commits into from Jun 1, 2022

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Apr 11, 2022

What does this PR do?

Implements the agent-side migration from sending span.destination.service.resource to span.service.target.type + span.service.target.name.

Impacted features:

  • dropped spans stats
  • compressed spans
  • User API ability to set a custom value for span.destination.service.resource
  • migration of all usages of span.destination.service.resource

The agent remains backwards compatible with existing APM servers:

  • only new fields are added to the protocol
  • existing fields values are inferred from the new internal data structure.

Checklist

@apmmachine
Copy link
Collaborator

apmmachine commented Apr 11, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-06-01T12:50:13.387+0000

  • Duration: 50 min 38 sec

Test stats 🧪

Test Results
Failed 0
Passed 2919
Skipped 22
Total 2941

💚 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!)

@SylvainJuge SylvainJuge changed the title Replace resource dest. with service target.{name.type} Replace/extend resource dest. with service target.{name.type} May 10, 2022
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 795 to 802
[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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should release not at the same day as the stack to make sure this didn't break cross-repo links

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to approve and let you decide what to do with not yet addressed comments, just used the wrong checkbox 🙂

@SylvainJuge
Copy link
Member Author

The comments should be fixed, I've opened elastic/apm#646 to enhance spec on other types of backends.

@SylvainJuge SylvainJuge merged commit 3365a0e into elastic:main Jun 1, 2022
APM-Agents (OLD) automation moved this from In Progress to Done Jun 1, 2022
@SylvainJuge SylvainJuge deleted the add-service-target branch June 1, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants