Implement outcome for transactions & spans by SylvainJuge · Pull Request #1613 · elastic/apm-agent-java

SylvainJuge · 2021-01-13T16:53:10Z

What does this PR do?

Fixes #1354

Fixes #1298
Fixes #1280

Implementation checklist

add outcome to the following plugins:

Extra:

restore & update gRPC test module with latest version (gRPC 1.35) fixes Restore latest gRPC tests #1298 and gRPC flaky test: "INTERNAL" where "OK" is expected #1280
add missing HTTP tags for OpenTracing HTTP client spans
Use a shared agent spec to ensure cross-agent consistent behavior (shared spec in WIP : Add transaction and span outcome gherkin feature file apm#402)

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

ghost · 2021-01-13T17:35:36Z

💚 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 #1613 updated
Start Time: 2021-02-09T08:50:13.326+0000
Duration: 43 min 41 sec
Commit: 54c4b43

Test stats 🧪

Test	Results
Failed	0
Passed	1815
Skipped	13
Total	1828

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	1815
Skipped	13
Total	1828

.../apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletTransactionHelper.java

…utcome

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Outcome.java

SylvainJuge · 2021-01-29T10:40:20Z

...plugins/apm-process-plugin/src/test/java/co/elastic/apm/agent/process/ProcessHelperTest.java

        verifyNoMoreInteractions(process); // we should not even use any method of process

+        // we have to opt-in to allow unknown outcome
+        reporter.checkUnknownOutcome(false);


@eyalkoren external process execution is one case where we might not always know the outcome if the process hasn't yet terminated. But I have to admit that it's quite likely to be a very narrow corner case.

I think this is a fair exception to make - if we never report unknown to terminated processes and always report unknown to unterminated processes it sounds reasonable.

eyalkoren

Looks great. The main comment is what we discussed about reflecting the revised spec where by default we set an outcome to spans, with some exceptions for specific span types.

Other than that, ResultUtilTest requires updating.

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java

apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java

eyalkoren

Once the tests pass, I am good with merging this PR.
If it's easy enough, switching to not allow unknown outcome for spans in the MockReporter and changing only for specific span types would be preferable as it means we cover all plugins, including future ones. If this complicates things, we can wait with that.

eyalkoren · 2021-02-09T05:27:42Z

apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java

    private final ObjectMapper objectMapper;
    private final boolean verifyJsonSchema;
-    private boolean checkUnknownOutcomes = false;
+    private boolean checkUnknownOutcomes = true;


...ent-plugins/apm-grpc/apm-grpc-plugin/src/main/java/co/elastic/apm/agent/grpc/GrpcHelper.java

apm-agent-core/src/test/resources/specs/outcome.feature

SylvainJuge added 10 commits December 10, 2020 15:34

add outcome + get/set & tests

8d755e2

reset outcome + test for it

2fc15be

get/set outcome

f7e342f

code cleanup

5ffee1c

add outcome as Boolean in public API

14652cd

use stand-alone enum + add user outcome

683d88c

modify public API

b781fff

fix javadoc links

48d3548

implement outcome for gRPC

16bd53d

restore latest gRPC test module

8768076

github-actions bot added the agent-java label Jan 13, 2021

felixbarny reviewed Jan 13, 2021

View reviewed changes

.../apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletTransactionHelper.java Outdated Show resolved Hide resolved

SylvainJuge added 9 commits January 14, 2021 13:33

handle HTTP spans, transactions & opentracing

32db189

test client & server OT bridge + fix client http

a01a4b5

add outcome to db spans

afaa67e

add outcome to ES and dubbo

ccd700a

add outcome to redis & ES-rest-client

7a2c9e9

add outcome to mongodb

d10ebeb

add outcome to quartz job transaction

fe5fefe

add outcome to scheduled annotations

6e0a2a4

minor code cleanup

360c0c8

SylvainJuge mentioned this pull request Jan 18, 2021

Improve process instrumentation for Java 9 #966

Open

SylvainJuge added 5 commits January 18, 2021 16:22

update impl. comments after package change

acef559

handle all msg plugins

75cdb94

Merge branch 'master' of github.com:elastic/apm-agent-java into add-o…

480c7cf

…utcome

add test for rabbitmq

cbf0f36

api Boolean -> Outcome

badf5be

SylvainJuge commented Jan 18, 2021

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java Show resolved Hide resolved

SylvainJuge commented Jan 18, 2021

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Outcome.java Show resolved Hide resolved

SylvainJuge added 13 commits January 20, 2021 13:22

code cleanup

1cfe8c0

make outcome not nullable

4d03e16

set outcome for process execution

7f3b686

enforce outcome with mock reporter

56c576f

test scheduler jobs outcomes

b2f7bbb

simplify grpc outcome mapping

ef7a474

ensure grpc outcome & result only set once

b2f7066

add optional outcome check on mock reporter

d0e8823

add opt-in check for unknown span/transaction outcome

d355072

fix test for opt-in outcome check

978100d

fix another source of grpc flakyness

0ca811d

another take at grpc flakyness

0fb8946

Merge branch 'master' into add-outcome

992b644

SylvainJuge commented Jan 29, 2021

View reviewed changes

SylvainJuge added 4 commits January 29, 2021 12:01

update documentation

b635ffc

add shared spec draft for outcome

2d76bc9

remove bookkeeper test check

ae99b78

update after shared spec PR update

d2933f6

eyalkoren reviewed Feb 8, 2021

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java Outdated Show resolved Hide resolved

apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java Outdated Show resolved Hide resolved

eyalkoren approved these changes Feb 8, 2021

View reviewed changes

SylvainJuge added 4 commits February 8, 2021 16:15

make http spec compliant

91668a1

fix api instrumentation

d358043

cleanup once unknown check enabled by default

8661e6d

allow for explicit unknown state

6891338

eyalkoren reviewed Feb 9, 2021

View reviewed changes

eyalkoren approved these changes Feb 9, 2021

View reviewed changes

...ent-plugins/apm-grpc/apm-grpc-plugin/src/main/java/co/elastic/apm/agent/grpc/GrpcHelper.java Show resolved Hide resolved

apm-agent-core/src/test/resources/specs/outcome.feature Show resolved Hide resolved

SylvainJuge added 2 commits February 9, 2021 09:15

Merge branch 'master' into add-outcome

033e6ab

align with shared spec PR for gRPC

54c4b43

SylvainJuge merged commit b099880 into elastic:master Feb 9, 2021

SylvainJuge deleted the add-outcome branch February 9, 2021 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement outcome for transactions & spans#1613

Implement outcome for transactions & spans#1613
SylvainJuge merged 51 commits intoelastic:masterfrom
SylvainJuge:add-outcome

SylvainJuge commented Jan 13, 2021 •

edited by zube bot

Loading

Uh oh!

ghost commented Jan 13, 2021 •

edited by ghost

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SylvainJuge Jan 29, 2021

Uh oh!

eyalkoren Jan 31, 2021

Uh oh!

eyalkoren left a comment

Uh oh!

Uh oh!

Uh oh!

eyalkoren left a comment

Uh oh!

eyalkoren Feb 9, 2021

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

SylvainJuge commented Jan 13, 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?

Implementation checklist

Checklist

Uh oh!

ghost commented Jan 13, 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 🧪

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SylvainJuge Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

eyalkoren Jan 31, 2021

Choose a reason for hiding this comment

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

eyalkoren Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SylvainJuge commented Jan 13, 2021 •

edited by zube bot

Loading

ghost commented Jan 13, 2021 •

edited by ghost

Loading