Added support for setting service name and version for a transaction via the public api by tobiasstadler · Pull Request #2451 · elastic/apm-agent-java

tobiasstadler · 2022-02-07T07:49:13Z

What does this PR do?

Adds support for setting service name and version for a transaction via the public api

Checklist

This is an enhancement of existing features, or a new feature in existing plugins
- I have updated CHANGELOG.asciidoc
- I have added tests that prove my fix is effective or that my feature works
- Added an API method or config option? Document in which version this will be introduced
- I have made corresponding changes to the documentation

…via the public api

github-actions · 2022-02-07T07:49:36Z

👋 @tobiasstadler Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

tobiasstadler · 2022-02-07T07:50:54Z

I will add unit tests once you decide this is the way to go (Manuel testing looks good).

ghost · 2022-02-07T07:57:21Z

💚 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-02-15T09:39:21.228+0000
Duration: 55 min 22 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2499
Skipped	16
Total	2515

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

tobiasstadler · 2022-02-08T10:03:21Z

@felixbarny @eyalkoren Are you both comfortable with this approach?

tobiasstadler · 2022-02-08T12:29:44Z

I added tests in 9be1a8a

tobiasstadler · 2022-02-08T14:38:31Z

I just saw you are already prepared the changeling for 1.29.0. So I guess this wont't make it into the release?

…ceinfo

tobiasstadler · 2022-02-09T11:39:17Z

Ok, hopefully 1.29.1 or 1.30.0 then

eyalkoren

Thanks for the PR @tobiasstadler !

The API types are only known within the API itself and not elsewhere within the agent, including the API plugin. Let's keep it this way. We don't want version assumptions that may make the agent backwards incompatible. Even though it is protected within your implementation by the fact that the getServiceInfoForClassLoader and setServiceInfoForClassLoader methods won't be matched, allowing this dependency may bite us in the future.

There are ways to do that within the API (like we do with multiple APIs through usage of internal methods that use Object in their signature), but I think the simplest would be to lose the ServiceInfo type.

One option is to get the service name and version through two APIs. It is not as neat, but it avoids the object allocation and it would be required within the internal API anyway if we want to keep the complete separation between API types and agent types.
Another option is to get rid of getServiceInfoForClassLoader altogether.
The fact that you added setServiceInfoForClassLoader is enough if the co.elastic.apm.api.Transaction interface has a useServiceInfoOf(ClassLoader) (instead of what you did in #2444) . You can reference from the javadoc of each method
to the other to give users hints how they can be used. A co.elastic.apm.api.Transaction#setServiceInfo(String serviceName, String serviceVersion) is still useful as well.

tobiasstadler · 2022-02-13T15:47:48Z

+        public static class AdviceClass {
+            @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+            public static void setServiceInfoForClassLoader(@Advice.Argument(0) @Nullable ClassLoader classLoader, @Advice.Argument(1) String serviceName, @Advice.Argument(2) @Nullable String serviceVersion) {
+                tracer.setServiceInfoForClassLoader(classLoader, ServiceInfo.of(serviceName, serviceVersion));


I wonder if we should check for serviceName being null

A null service name will have no effect here - it won't be mapped and not even cause allocation of a ServiceInfo object.
However, I wonder if we should allow null service version going forward, now that #1726 is merged.
I think providing a service name without a version should be considered a bug, because it means we will use the global service version for a manually set service name, and there's no real reason why those would be related.
@felixbarny WDYT? Should we only allow overriding these two values together?

I think its is fine to set a service name without a version. The global service version won't be used in such a case. But there service version won't be serialized and the apm-server also won't use the global service version.

You mean - currently APM Server ignores the global version when we use a service name in transaction/metric context?

Sorry, my bad. This logic is only implemented for metrics. I created elastic/apm-server#7281 to fix this for transactions also.

eyalkoren

Looks good, thanks!
Once we set our minds about how we handle null values, please update the tests accordingly to verify that.

eyalkoren · 2022-02-14T07:34:34Z

+        public static class AdviceClass {
+            @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+            public static void setServiceInfoForClassLoader(@Advice.Argument(0) @Nullable ClassLoader classLoader, @Advice.Argument(1) String serviceName, @Advice.Argument(2) @Nullable String serviceVersion) {
+                tracer.setServiceInfoForClassLoader(classLoader, ServiceInfo.of(serviceName, serviceVersion));


A null service name will have no effect here - it won't be mapped and not even cause allocation of a ServiceInfo object.
However, I wonder if we should allow null service version going forward, now that #1726 is merged.
I think providing a service name without a version should be considered a bug, because it means we will use the global service version for a manually set service name, and there's no real reason why those would be related.
@felixbarny WDYT? Should we only allow overriding these two values together?

Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

eyalkoren

Thanks for following up @tobiasstadler !
Please just add tests that reflect our decision on using null values (I am approving anyway).

eyalkoren · 2022-02-14T14:09:00Z

/test

felixbarny

One optional suggestion to bring in some of the improvements from #2444. Otherwise, LGTM.

felixbarny · 2022-02-14T16:41:54Z

/run elasticsearch-ci/docs

tobiasstadler · 2022-02-16T07:19:42Z

Thank You!

eyalkoren · 2022-02-16T07:23:41Z

Thank you for your great collaboration ❤️

Added support for setting service name and version for a transaction …

fdb7dcc

…via the public api

github-actions Bot added agent-java community Issues and PRs created by the community triage labels Feb 7, 2022

Added elastic#2451 to the changelog

3b75b25

tobiasstadler mentioned this pull request Feb 7, 2022

Support for setting the application class loader for a transaction via the public api #2444

Closed

5 tasks

tobiasstadler added 2 commits February 8, 2022 13:10

Merge branch 'main' into public-api-setserviceinfo

01255ac

Added tests

9be1a8a

Merge branch 'main' into public-api-setserviceinfo

3451052

tobiasstadler added 2 commits February 9, 2022 08:46

Merge branch 'elastic:main' into public-api-setserviceinfo

69098b4

Merge remote-tracking branch 'upstream/main' into public-api-setservi…

cf91e91

…ceinfo

eyalkoren suggested changes Feb 13, 2022

View reviewed changes

Review by @eyalkoren

3a1cb68

tobiasstadler commented Feb 13, 2022

View reviewed changes

Comment thread .../apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentation.java Outdated

tobiasstadler commented Feb 13, 2022

View reviewed changes

eyalkoren reviewed Feb 14, 2022

View reviewed changes

1.29.0 is already released

a1b76ed

Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

eyalkoren approved these changes Feb 14, 2022

View reviewed changes

eyalkoren requested a review from felixbarny February 14, 2022 14:08

felixbarny approved these changes Feb 14, 2022

View reviewed changes

Comment thread .../apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/TransactionInstrumentation.java Outdated

Only set service name/version if the service name is not empty

4d20030

eyalkoren merged commit d48e1d6 into elastic:main Feb 16, 2022

tobiasstadler deleted the public-api-setserviceinfo branch February 16, 2022 07:19

Uh oh!

Conversation

tobiasstadler commented Feb 7, 2022 • edited by eyalkoren Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Uh oh!

github-actions Bot commented Feb 7, 2022

Uh oh!

tobiasstadler commented Feb 7, 2022

Uh oh!

ghost commented Feb 7, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

tobiasstadler commented Feb 8, 2022

Uh oh!

tobiasstadler commented Feb 8, 2022

Uh oh!

tobiasstadler commented Feb 8, 2022

Uh oh!

tobiasstadler commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasstadler Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

eyalkoren Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

tobiasstadler Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

tobiasstadler Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

eyalkoren Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

eyalkoren commented Feb 14, 2022

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixbarny commented Feb 14, 2022

Uh oh!

tobiasstadler commented Feb 16, 2022

Uh oh!

eyalkoren commented Feb 16, 2022

Uh oh!

tobiasstadler commented Feb 7, 2022 •

edited by eyalkoren

Loading

ghost commented Feb 7, 2022 •

edited by ghost

Loading

tobiasstadler commented Feb 9, 2022 •

edited

Loading

tobiasstadler Feb 14, 2022 •

edited

Loading