feat: span statistics -- ensure dropped span objects are still created #2694

astorm · 2022-05-15T06:01:40Z

Part of #2302 (2 of 3):

This intent of this PR is to ensure that in cases when a span will be dropped that the span is still created (matching the behavior of the java agent). This is necessary for the span statistics work, as the collected statistics includes capturing the sum of time for dropped spans -- meaning that even a dropped span needs to start and end.

Prior to this work the node agent "dropping" a span meant that the span would not be created. Now, the span object is created but the span's context object has its recorded property set to false. Non-recorded spans are then excluded from sending, but otherwise have a full start/end lifecycle.

The next (and final) step after this PR is to implement the logic for capturing the dropped span statistics and sending them on to APM Server.

Checklist

Implement code
Edit tests
~~[ ] Update TypeScript typings~~
~~[ ] Update documentation~~
~~[ ] Add CHANGELOG.asciidoc entry~~

apmmachine · 2022-05-15T06:21:39Z

💚 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-05-19T20:36:09.170+0000
Duration: 27 min 3 sec

Test stats 🧪

Test	Results
Failed	0
Passed	254873
Skipped	0
Total	254873

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

astorm · 2022-05-15T06:57:30Z

lib/instrumentation/transaction.js

@@ -153,8 +149,15 @@ Transaction.prototype.createSpan = function (...args) {
    return null
  }

+  const span = new Span(this, ...args, opts)
+
+  if (this._builtSpans >= this._agent._conf.transactionMaxSpans) {


Note: The meaning of _buildSpans ends up changing slightly in this new world. The _buildSpans property is always incremented regardless of whether a span will be dropped or not. To get the number of started spans we subtract the number of dropped spans from built spans (see below)

test/opentelemetry-bridge/fixtures.test.js

astorm · 2022-05-15T16:54:07Z

lib/instrumentation/transaction.js

@@ -172,7 +175,7 @@ Transaction.prototype.toJSON = function () {
    sampled: this.sampled,
    context: undefined,
    span_count: {
-      started: this._builtSpans
+      started: this._builtSpans - this._droppedSpans


The _builtSpans property now tracks all span objects that were created. To se the number of spans started we subtract the number dropped. (see above)

astorm · 2022-05-15T16:55:06Z

lib/opentelemetry-bridge/OTelTracer.js

@@ -74,7 +74,7 @@ class OTelTracer {
      }
      newTransOrSpan = trans.createSpan(name, spanOpts)

-      // There might be no span, e.g. if transactionMaxSpans was hit. We have
+      // There might be no span, e.g. if the span is an exit span. We have


Note: transactionMaxSpans being reached now creates a span -- but sets its recorded value to false.

astorm · 2022-05-16T15:24:56Z

jenkins run the tests please

trentm

This looks mostly good.

I think it is worth discussing the behaviour change with other agent teams.
Then if we are good with that change, I'd like to update or remove the OTel Bridge "hit-transaction-max-spans.js" test case, because it will no longer be testing what it was meant to be testing.

I think it might be worth creating a separate ticket to investigate the performance impact of this change when the agent does hit transactionMaxSpans. (Or perhaps we just stay aware of it, if it comes up.) For example, transactionMaxSpans is no longer any help in reducing the CPU load of calculating breakdown metrics, or of captureSpanStackTraces if that happens to be enabled (hopefully not).

test/opentelemetry-bridge/fixtures.test.js

astorm · 2022-05-17T02:12:19Z

jenkins run the tests please

…eSpan-returns-null.js to test the intended OTelTracer.startSpan case where the internal createSpan API returns a null

trentm

See the 'addEndedSpan' comment below.

I also think it would good to determine if breakdown metrics calculation should be skipped for these non-recording spans (for both correctness and perf), and if gatherStackTrace could be skipped for these non-recording spans (for perf). Both of these are in Span.prototype.end. However, I'm not following up on those.

lib/instrumentation/index.js

The change away from `NODE_VERSION` in #2627 surprisingly broke running the benchmarks. `NODE_VERSION` is used by nvm and exporting it (?) makes a difference?

…ue with github deps (#2696) [email protected] (and only that version) has a github dep: "@types/mysql": "types/mysql", Attempting to install that version with npm v6 (the npm in node v10, v12, and v14) hits npm/cli#4896 which results in an install so slow that is hits the default 2 minute 'npm install' timeout in the `tav` tool.

This adds a 'links' option to `createTransaction` and `createSpan` APIs for specifying span links. apm.startSpan('name', { links: [ ... ] }) https://github.com/elastic/apm/blob/main/specs/agents/span-links.md Span links support is added to the OTel Bridge as well: tracer.startSpan('name', { links: [ ... ] }) This adds a `traceContinutationStrategy` configuration option to allow some control over how the APM Agent uses incoming trace-context headers for context propagation. https://github.com/elastic/apm/blob/main/specs/agents/trace-continuation.md This also fixes a whitespace parsing issue in TraceState. Closes: #2592 Closes: #2673 Closes: #2554 Obsoletes: #2555

AFAIK the "compatible runtimes" flag on Lambda layers is just advisory.

Co-authored-by: apmmachine <[email protected]>

This avoids the name collision with the "exitSpan" option when creating a span in the public API. Closes: #2680

The old 'hapi' package was itself deprecated about 2 years ago. The APM agent now deprecates instrumenting it. The instrumentation code remains, but it is no longer tested and will be removed in the next major. Note that '@hapi/hapi' is still supported, this is just about the old package name and versions. Any versions starting with 17.9.0, 18.2.0, 19 and later are the newer '@hapi/hapi'. Refs: #2691

Co-authored-by: apmmachine <[email protected]>

Alan Storm added 4 commits May 13, 2022 09:29

feat: in progress

b2edc03

feat: remove logging

7a1cf0e

feat: make max span test fit

4d886b0

fix: lint

84fa951

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

fix: lint

f6a5afa

astorm commented May 15, 2022

View reviewed changes

test/opentelemetry-bridge/fixtures.test.js Show resolved Hide resolved

astorm commented May 15, 2022

View reviewed changes

astorm marked this pull request as ready for review May 15, 2022 16:56

Alan Storm added 2 commits May 15, 2022 10:09

fix: lint

6736d5b

fix: correcting comment

7169ef3

astorm requested a review from trentm May 16, 2022 15:33

trentm reviewed May 16, 2022

View reviewed changes

test/opentelemetry-bridge/fixtures.test.js Show resolved Hide resolved

trentm added 2 commits May 17, 2022 09:19

replace hit-transaction-max-spans.js OTel Bridge test case with creat…

e0825e5

…eSpan-returns-null.js to test the intended OTelTracer.startSpan case where the internal createSpan API returns a null

fix lint and remove debugging printf

e0e961c

trentm reviewed May 17, 2022

View reviewed changes

lib/instrumentation/index.js Outdated Show resolved Hide resolved

trentm and others added 8 commits May 19, 2022 13:28

test: fix running benchmarks (#2693)

f57cc51

The change away from `NODE_VERSION` in #2627 surprisingly broke running the benchmarks. `NODE_VERSION` is used by nvm and exporting it (?) makes a difference?

feat: add "nodejs16.x" lambda runtime to our compatible runtimes (#2697)

9261276

AFAIK the "compatible runtimes" flag on Lambda layers is just advisory.

synchronize json schema specs (#2702)

44e56dd

Co-authored-by: apmmachine <[email protected]>

refactor: RunContext.exitSpan() -> .leaveSpan() (#2703)

d31e33c

This avoids the name collision with the "exitSpan" option when creating a span in the public API. Closes: #2680

chore(deps-dev): bump pug from 2.0.4 to 3.0.1 (#2690)

ac19339

apmmachine and others added 3 commits May 19, 2022 13:28

synchronize json schema specs (#2704)

eec972b

Co-authored-by: apmmachine <[email protected]>

fix: move isRecoded check

152df0c

Merge branch 'main' into astorm/span-stats-create-dropped-span

3e6c99e

astorm requested a review from trentm May 20, 2022 02:21

astorm mentioned this pull request May 20, 2022

feat: dropped span stats #2707

Merged

3 tasks

trentm approved these changes May 20, 2022

View reviewed changes

astorm merged commit 3c4e18d into main May 20, 2022

astorm deleted the astorm/span-stats-create-dropped-span branch May 20, 2022 17:06

trentm mentioned this pull request Jan 18, 2023

investigate trace-context propagation after transaction_max_spans is hit #3109

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: span statistics -- ensure dropped span objects are still created #2694

feat: span statistics -- ensure dropped span objects are still created #2694

astorm commented May 15, 2022 •

edited

Loading

apmmachine commented May 15, 2022 •

edited

Loading

Build stats

Test stats 🧪

astorm May 15, 2022 •

edited

Loading

astorm May 15, 2022

astorm May 15, 2022

astorm commented May 16, 2022

trentm left a comment

astorm commented May 17, 2022

trentm left a comment

feat: span statistics -- ensure dropped span objects are still created #2694

feat: span statistics -- ensure dropped span objects are still created #2694

Conversation

astorm commented May 15, 2022 • edited Loading

Checklist

apmmachine commented May 15, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

astorm May 15, 2022 • edited Loading

Choose a reason for hiding this comment

astorm May 15, 2022

Choose a reason for hiding this comment

astorm May 15, 2022

Choose a reason for hiding this comment

astorm commented May 16, 2022

trentm left a comment

Choose a reason for hiding this comment

astorm commented May 17, 2022

trentm left a comment

Choose a reason for hiding this comment

astorm commented May 15, 2022 •

edited

Loading

apmmachine commented May 15, 2022 •

edited

Loading

astorm May 15, 2022 •

edited

Loading