Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

trentm
Copy link
Member

@trentm trentm commented Nov 25, 2020

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

Fixes #1889

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
@apmmachine
Copy link
Contributor

apmmachine commented Nov 25, 2020

💔 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
Member Author

trentm commented Feb 4, 2021

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
Copy link
Member Author

trentm commented Sep 30, 2021

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

@trentm
Copy link
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
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
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
2 participants