Issue 1699 extend destination field by videnkz · Pull Request #1788 · elastic/apm-agent-java

videnkz · 2021-04-26T12:03:44Z

What does this PR do?

closes #1699

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

…-destination-field

ghost · 2021-04-26T12:07:11Z

💚 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

Build Cause: Pull request #1788 updated
Start Time: 2021-06-23T14:48:29.769+0000
Duration: 51 min 25 sec
Commit: 14805b3

Test stats 🧪

Test	Results
Failed	0
Passed	2286
Skipped	19
Total	2305

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	2286
Skipped	19
Total	2305

felixbarny · 2021-04-27T06:18:15Z

Hey @kananindzya 👋
Thanks for the PR. However, we're currently re-thinking how the API should look like given that the destination.service.name and destination.service.type are not actually used. More info here: elastic/apm#424 (comment)

…-destination-field

…elds from serializaiton. Deleted this field from public api.

apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java

apm-agent-api/src/main/java/co/elastic/apm/api/TransactionImpl.java

eyalkoren · 2021-05-04T09:12:18Z

Blocked until elastic/apm#436 is finalized

eyalkoren

Thanks (again) @kananindzya !
Some changes required, mostly minor ones.
Please add tests to verify that the destination.service.resource provide through the API always take precedence, considering that it can be set before or after the resource was automatically set.

apm-agent-api/src/main/java/co/elastic/apm/api/TransactionImpl.java

apm-agent-api/src/main/java/co/elastic/apm/api/AbstractSpanImpl.java

apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java

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

...ins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/SpanInstrumentationTest.java

eyalkoren · 2021-06-09T12:36:33Z

@kananindzya would you like me to take over and apply these minor fixes?
I can make a PR on this PR if you want

videnkz · 2021-06-09T12:38:06Z

@kananindzya would you like me to take over and apply these minor fixes?
I can make a PR on this PR if you want

Hi @eyalkoren, after 30 min I will send changes.

…-destination-field

eyalkoren

@kananindzya Thanks for the fixes! 🙏

There were some required fixes, including for tests and updating according to the latest spec changes, so I did these changes myself.
If you disagree with something in my latest commits, please let me know.
Thanks!

eyalkoren · 2021-06-16T13:19:41Z

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

+                                                 @Advice.Argument(1) int port) {
+            if (context instanceof Span) {
+                SpanContext spanContext = ((Span) context).getContext();
+                if (address != null && !address.isBlank()) {


isBlank() is Java 11, we cannot use it

eyalkoren · 2021-06-17T11:45:48Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/Destination.java


        public boolean hasContent() {
-            return resource.length() > 0 || name.length() > 0 || type != null;
+            return resource.length() > 0;


userResource should be taken into account as well

eyalkoren · 2021-06-17T11:46:15Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/Destination.java

         */
        private final StringBuilder resource = new StringBuilder();

+        private final StringBuilder userResource = new StringBuilder();


Must be reset in resetState()

felixbarny · 2021-06-21T08:27:15Z

Instead of duplicating the properties for user-supplied values, would it be possible to either

Only override existing values if they are user-supplied (no additional properties needed)
Mark properties that have been user-supplied with a boolean flag (similar to the span name priority)

…-destination-field

eyalkoren · 2021-06-23T15:46:58Z

@elasticmachine run elasticsearch-ci/docs

videnkz added 3 commits April 25, 2021 16:36

feature added set destination address and service methods for public api

28086e9

Merge remote-tracking branch 'upstream/master' into issue-1699-extend…

6a81c1a

…-destination-field

added instrumentation for setDestinationAddress, setDestinationService

a4a563e

github-actions bot added the agent-java label Apr 26, 2021

added more tests

9054031

videnkz added 3 commits April 27, 2021 14:54

Merge remote-tracking branch 'upstream/master' into issue-1699-extend…

dd9bb1f

…-destination-field

Merge remote-tracking branch 'upstream/master' into issue-1699-extend…

0b81e3a

…-destination-field

mark Destination.Service name and type as deprecated. Deleted this fi…

41be722

…elds from serializaiton. Deleted this field from public api.

felixbarny reviewed Apr 29, 2021

View reviewed changes

fixes according to comments

ac22962

eyalkoren added the blocked label May 4, 2021

eyalkoren mentioned this pull request Jun 6, 2021

added support for adding span destination service info to public api #1551

Closed

eyalkoren suggested changes Jun 7, 2021

View reviewed changes

Merge branch 'master' into issue-1699-extend-destination-field

c8afc98

videnkz and others added 6 commits June 9, 2021 18:54

fixes according to comments

f521832

added userResource to Destination.Service

2bcfcaf

Merge branch 'master' into issue-1699-extend-destination-field

8ee6a2b

Adjusting to updated spec

abaa5d7

Merge remote-tracking branch 'upstream/master' into issue-1699-extend…

3e6410b

…-destination-field

Adjusting API test

69178ba

eyalkoren reviewed Jun 20, 2021

View reviewed changes

Adding to docs and CHANGELOG

f318d39

eyalkoren approved these changes Jun 20, 2021

View reviewed changes

eyalkoren requested a review from SylvainJuge June 20, 2021 12:14

eyalkoren and others added 4 commits June 21, 2021 16:36

Applying review suggestion

69360a3

Merge remote-tracking branch 'upstream/master' into issue-1699-extend…

45e11eb

…-destination-field

add few extra destination tests

806193c

minor renaming

5af4069

SylvainJuge approved these changes Jun 22, 2021

View reviewed changes

SylvainJuge and others added 3 commits June 22, 2021 12:12

Merge branch 'master' into issue-1699-extend-destination-field

fbce153

Merge branch 'master' into issue-1699-extend-destination-field

b5b7a4a

Merge branch 'master' into issue-1699-extend-destination-field

14805b3

eyalkoren merged commit c751adc into elastic:master Jun 23, 2021

This was referenced Jul 11, 2021

Added support for setting destination.address/port on JMS (client) spans who use ActiveMQ Artemis as underlying technology #1905

Closed

Added support for setting the framework using the public api (#1908) #1909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1699 extend destination field#1788

Issue 1699 extend destination field#1788
eyalkoren merged 23 commits intoelastic:masterfrom
videnkz:issue-1699-extend-destination-field

videnkz commented Apr 26, 2021 •

edited by zube bot

Loading

ghost commented Apr 26, 2021 •

edited by ghost

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

felixbarny commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

eyalkoren commented May 4, 2021

eyalkoren left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eyalkoren commented Jun 9, 2021 •

edited

Loading

videnkz commented Jun 9, 2021

eyalkoren left a comment

eyalkoren Jun 16, 2021

eyalkoren Jun 17, 2021

eyalkoren Jun 17, 2021

felixbarny commented Jun 21, 2021

eyalkoren commented Jun 23, 2021

Labels

4 participants

Conversation

videnkz commented Apr 26, 2021 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

ghost commented Apr 26, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

felixbarny commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

eyalkoren commented May 4, 2021

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!

Uh oh!

eyalkoren commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

videnkz commented Jun 9, 2021

eyalkoren left a comment

Choose a reason for hiding this comment

eyalkoren Jun 16, 2021

Choose a reason for hiding this comment

eyalkoren Jun 17, 2021

Choose a reason for hiding this comment

eyalkoren Jun 17, 2021

Choose a reason for hiding this comment

felixbarny commented Jun 21, 2021

eyalkoren commented Jun 23, 2021

Labels

4 participants

videnkz commented Apr 26, 2021 •

edited by zube bot

Loading

ghost commented Apr 26, 2021 •

edited by ghost

Loading

eyalkoren commented Jun 9, 2021 •

edited

Loading