feat: introduce "exit spans" and mark S3 spans as exit spans by trentm · Pull Request #2552 · elastic/apm-agent-nodejs

trentm · 2022-01-28T20:49:40Z

Per https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans.md#exit-spans
the result is that HTTP child spans of S3 spans are no longer captured.

Closes: #2125

Overview

There is a boolean exitSpan option to the startSpan() APIs now, which we'll eventually use in our instrumentations. For now I've just added it to the s3 instrumentation to get a feel. When creating a new span, if its parent is an exitSpan then the span won't be created.

I manually tested that context-propagation still works, but don't have a test case for that, because it is a pain in the S3 tests. I plan to add tests for context-propagation still working in #2000 (because I already have a MockES in tests that I can easily use to test the traceparent et al headers).

In subsequent PRs we can work through adding , { exitSpan: true } to the relevant instrumentations.

Checklist

Per https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans.md#exit-spans the result is that HTTP child spans of S3 spans are no longer captured. Closes: #2125

ghost · 2022-01-28T21:17:14Z

💚 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

Reason: null
Start Time: 2022-01-28T21:56:53.688+0000
Duration: 30 min 9 sec
Commit: bf01aef

Test stats 🧪

Test	Results
Failed	0
Passed	243533
Skipped	0
Total	243533

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules
run benchmark tests : Run the benchmark test only.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

trentm · 2022-01-28T21:39:31Z

I'm experimenting a bit with adding some span.context.http. For example:

    {
        "span": {
            "name": "S3 GetObject elasticapmtest-bucket-20220128213346",
            "type": "storage",
            "id": "4efe60d5f914412c",
            "transaction_id": "766ebd597ecbc0dd",
            "parent_id": "766ebd597ecbc0dd",
            "trace_id": "32663fe436de85979ae7426674892ff4",
            "subtype": "s3",
            "action": "GetObject",
            "timestamp": 1643405628127090,
            "duration": 270.786,
            "context": {
                "http": {
                    "method": "GET",
                    "status_code": 304,
                    "url": "https://s3.us-east-2.amazonaws.com/",
                    "response": {
                        "encoded_body_size": 0,
                        "headers": {
                            "x-amz-id-2": "qN15KVu9Soe42YSRcU1SNJ8z5XByyC85fS1GDvpZS9BnJTlyHbROwebKhq3LsfuHsFhNNMZGRZU=",
                            "x-amz-request-id": "G4TW207TJAMB962W",
                            "date": "Fri, 28 Jan 2022 21:33:49 GMT",
                            "last-modified": "Fri, 28 Jan 2022 21:33:48 GMT",
                            "etag": "\"fd33e2e8ad3cb1bdd3ea8f5633fcf5c7\"",
                            "server": "AmazonS3"
                        }
                    }
                },
                "destination": {
                    "service": {
                        "name": "s3",
                        "type": "storage",
                        "resource": "elasticapmtest-bucket-20220128213346"
                    },
                    "address": "s3.us-east-2.amazonaws.com",
                    "port": 443,
                    "cloud": {
                        "region": "us-east-2"
                    }
                }
            },
            "sync": false,
            "outcome": "success",
            "sample_rate": 1
        }
    }

However, I am inclined to not include:

method: Not that helpful.
url: Mostly redundant with context.destination.address.
response.headers: A lot of added size for uncertain utility. The inclusion of Amazon's request ID headers might be worth it.

…me of this info might be helpful

trentm · 2022-01-28T21:51:42Z

run module tests for aws-sdk

trentm · 2022-01-28T21:56:06Z

The TAV test run for aws-sdk is getting out of hand again. They are a lot of the slowest TAV runs:

% ./dev-utils/jenkins-build-slow-steps.sh https://apm-ci.elastic.co/job/apm-agent-nodejs/job/apm-agent-nodejs-mbp/job/main/18/
...
1359145 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "bluebird" "12"
1361483 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "graphql" "12"
1363566 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "aws-sdk" "8"
1386958 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "aws-sdk" "14"
1402172 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "pg" "12"
1405902 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "hapi" "12"
1421825 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "knex" "12"
1422159 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "knex" "10"
1458659 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "knex" "14"
1505502 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "aws-sdk" "12"
1600148 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "apollo-server-express" "14"
1625149 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "apollo-server-express" "12"
1678785 FINISHED SUCCESS .ci/scripts/test.sh -b "release" -t "aws-sdk" "10"

Current TAV runs are testing all these release versions:

-- package versions matching "2.858.0 || 2.889.0 || 2.920.0 || 2.951.0 || 2.982.0 || 2.1013.0 || 2.1016.0 || >2.1016.0 <3": 2.858.0, 2.889.0, 2.920.0, 2.951.0, 2.982.0, 2.1013.0, 2.1016.0, 2.1017.0, 2.1018.0, 2.1019.0, 2.1020.0, 2.1021.0, 2.1022.0, 2.1023.0, 2.1024.0, 2.1025.0, 2.1026.0, 2.1027.0, 2.1028.0, 2.1029.0, 2.1030.0, 2.1031.0, 2.1032.0, 2.1033.0, 2.1034.0, 2.1035.0, 2.1036.0, 2.1037.0, 2.1038.0, 2.1039.0, 2.1040.0, 2.1041.0, 2.1042.0, 2.1043.0, 2.1044.0, 2.1045.0, 2.1046.0, 2.1047.0, 2.1048.0, 2.1049.0, 2.1050.0, 2.1051.0, 2.1052.0, 2.1053.0, 2.1054.0, 2.1055.0, 2.1056.0, 2.1057.0, 2.1058.0, 2.1059.0, 2.1060.0, 2.1061.0, 2.1062.0, 2.1063.0, 2.1064.0, 2.1065.0, 2.1066.0

I'll update the versions to test:

% ./dev-utils/aws-sdk-tav-versions.sh
  # Test v2.858.0, every N=41 of 209 releases, and current latest.
  versions: '2.858.0 || 2.899.0 || 2.940.0 || 2.981.0 || 2.1022.0 || 2.1063.0 || 2.1066.0 || >2.1066.0 <3'

trentm · 2022-01-28T21:56:13Z

run module tests for aws-sdk

trentm · 2022-01-28T22:35:24Z

lib/instrumentation/index.js

    return null
  }
-  return this._runCtxMgr.active().currTransaction() || null
+  return this._runCtxMgr.active().currTransaction()


REVIEW NOTE: This was redundant. RunContext.currTransaction() already defaults to null if there isn't a transaction.

feat: introduce "exit spans" and mark S3 spans as exit spans

6cfb2ef

Per https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans.md#exit-spans the result is that HTTP child spans of S3 spans are no longer captured. Closes: #2125

trentm self-assigned this Jan 28, 2022

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jan 28, 2022

add http context to the S3 spans, now that the http spans are gone so…

15d305f

…me of this info might be helpful

update TAV versions for aws-sdk to something more reasonable

bf01aef

trentm marked this pull request as ready for review January 28, 2022 22:33

trentm requested a review from astorm January 28, 2022 22:33

trentm commented Jan 28, 2022

View reviewed changes

astorm approved these changes Jan 29, 2022

View reviewed changes

Merge branch 'main' into trentm/issue2125-exit-spans-drop-S3-HTTP-spans

b5a154b

trentm merged commit 520a901 into main Jan 31, 2022

trentm deleted the trentm/issue2125-exit-spans-drop-S3-HTTP-spans branch January 31, 2022 15:57

trentm mentioned this pull request Feb 4, 2022

fix: apm.createSpan() with no args would create bogus span.name #2563

Merged

2 tasks

This was referenced Sep 14, 2022

[META 448] Auto-infer destination.service.resource and adapt public API #2103

Closed

feat: exit spans, infer destination.service.resource #2458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce "exit spans" and mark S3 spans as exit spans#2552

feat: introduce "exit spans" and mark S3 spans as exit spans#2552
trentm merged 4 commits intomainfrom
trentm/issue2125-exit-spans-drop-S3-HTTP-spans

trentm commented Jan 28, 2022 •

edited

Loading

Uh oh!

ghost commented Jan 28, 2022 •

edited by ghost

Loading

Build stats

Test stats 🧪

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

trentm commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checklist

Uh oh!

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

🤖 GitHub comments

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm commented Jan 28, 2022

Uh oh!

trentm Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trentm commented Jan 28, 2022 •

edited

Loading

ghost commented Jan 28, 2022 •

edited by ghost

Loading