Skip to content

fix: spans started in same async op should be children#1890

Closed
trentm wants to merge 1 commit intomasterfrom
trentm/sync-span-child
Closed

fix: spans started in same async op should be children#1890
trentm wants to merge 1 commit intomasterfrom
trentm/sync-span-child

Conversation

@trentm
Copy link
Copy Markdown
Member

@trentm trentm commented Nov 25, 2020 •
edited by zube bot
Loading

This commit is just a start. There are still tests and discussion to be
had.

Fixes #1889

@trentm
fix: spans started in same async op should be children
6c30661 
This commit is just a start. There are still tests and discussion to be
had.

Fixes #1889
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Nov 25, 2020
@trentm trentm self-assigned this Nov 25, 2020
@ghost
Copy link
Copy Markdown

ghost commented Nov 25, 2020 •
edited by ghost
Loading

💔 Tests Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-27T00:59:21.722+0000

  • Duration: 11 min 17 sec

  • Commit: 6c30661

Test stats 🧪

Test Results
Failed 18
Passed 0
Skipped 0
Total 18

Test errors 18

Expand to view the tests failures

> Show only the first 10 test failures

Test / Node.js-12.0-async-hooks-false / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-3a1dc6b5-14f9-4c59-b134-d68d7f60cfab/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-8 / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-787628e1-a0c0-4594-ae3d-ae3e0e51bcf0/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-12 / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-cadef9d2-b39f-4091-9264-9a4e3d0b0d39/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-13-async-hooks-false / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-25a7eb8c-9dcf-4b12-8bf5-75462a258ed8/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-8.6 / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-070f02bf-dc34-4484-bc0d-62df4f80b65a/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-8-async-hooks-false / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-6e62d009-39a1-4f40-9e53-fa3992ff3dcb/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-10.0-async-hooks-false / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-584ade51-0744-483b-a201-636b43aba9c3/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-14.0-async-hooks-false / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-716435f6-2d34-44f2-a462-da4364a2d6be/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-10 / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-b9cac928-81a6-4b54-a2eb-0d43b85b4300/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Test / Node.js-10-async-hooks-false / [empty] – *-output-junit.xml
  • no error details
    • Expand to view the stacktrace

     Test report file /var/lib/jenkins/workspace/PR-1890-121-60dcd03c-768f-4322-85a0-416fdb0d95a0/src/github.com/elastic/apm-agent-nodejs/*-output-junit.xml was length 0

Steps errors 38

Expand to view the steps failures

Show only the first 10 steps failures

Run Tests
  • Took 0 min 19 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8"
Run Tests
  • Took 0 min 1 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8"
Run Tests
  • Took 1 min 54 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8"
Run Tests
  • Took 0 min 1 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8"
Run Tests
  • Took 1 min 51 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8.6"
Run Tests
  • Took 0 min 1 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8.6"
Run Tests
  • Took 1 min 47 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8.6"
Run Tests
  • Took 0 min 1 sec . View more details here
  • Description: .ci/scripts/test.sh -b "release" -t "" "8.6"
Prepare services
  • Took 0 min 18 sec . View more details here
  • Description: .ci/scripts/windows/prepare-test.sh 14
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: hudson.AbortException: script returned exit code 1

🤖 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.

@trentm
Copy link
Copy Markdown
Member Author

trentm commented Feb 4, 2021 •
edited
Loading

Here is a case, from @dgieselaar, that shows this patch is insufficient.

// ... apm and app (express) setup ...
app.get('/nspans-dario', (req, res) => {
  var firstSpan = apm.startSpan('first-span', 'type', 'subtype', 'action')
  process.nextTick(function ( ) {
    firstSpan.end()
    var secondSpan = apm.startSpan('second-span', 'type', 'subtype', 'action')
    secondSpan.end()
    res.end('done')
  })
})

The transaction/span hierarchy resulting from a call to this is:

    transaction
        span first-span
            span second-span

where arguably we want:

    transaction
        span first-span
        span second-span

We are no longer in the same async op here, so technically wasn't what this ticket was first considering. However, this is likely a more common case and we should consider it.

Take a look at Dario's draft PR here: #1964

@trentm trentm mentioned this pull request Feb 5, 2021
Closed
@trentm
Copy link
Copy Markdown
Member Author

trentm commented Sep 30, 2021

This can be closed if/when #2181 goes in.

@trentm trentm mentioned this pull request Oct 16, 2021
Merged
5 tasks
@trentm
Copy link
Copy Markdown
Member Author

trentm commented Oct 27, 2021

I believe this is fixed by #2181

@trentm trentm closed this Oct 27, 2021
@trentm trentm deleted the trentm/sync-span-child branch October 27, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@trentm trentm

Labels

agent-nodejs Make available for APM Agents project planning.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

startSpan('A'); startSpan('B') in the same async op: B should be a child of A, but isn't

1 participant

@trentm