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

Span start/end after parent and trans end should be allowed #635

Merged
merged 1 commit into from
May 3, 2022

Conversation

trentm
Copy link
Member

@trentm trentm commented Apr 21, 2022

An APM agent shouldn't prevent the creation/start of a span after its parent or its containing transaction has ended.

FWIW, the Java APM agent (I'm told) already allows span creation and end after its parent and transaction have ended.

Motivation

It is possible (at least in Node.js) to have a span creation/start happen after its containing transaction has already ended. With the OTel API this can happen like this:

const otel = require('@opentelemetry/api')
const tracer = otel.trace.getTracer('play')

// Create an OTel span "s1" with the OTel API. Here we create Transaction "s1".
const s1 = tracer.startSpan('s1')
const ctx1 = otel.trace.setSpan(otel.context.active(), s1)

// The edge case: Create a child of "s1" after s1 has *ended*.
// - When using the OTel SDK: This works. s4 is a child of s1.
// - When using the Elastic OTel Bridge: This results in an *attempt* to create
//   a child Span of s1, but that returns `null` with the debug log:
//        DEBUG: transaction already ended - cannot build new span
s1.end()
const s4 = tracer.startSpan('s4', {}, ctx1)
s4.end()

This "transaction already ended - cannot build new span" limitation in the current Node.js APM agent dates back to v1 of the APM Server intake API when all of a transaction's spans had to be reported with the transaction -- and that was done on transaction end. The v2 intake API no longer requires this. elastic/apm-agent-nodejs#2653 will be removing this restriction in the Node.js APM agent.

checklist

  • May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
    • Yes
      • Add a section to the spec how agents should apply sanitization (such as sanitize_field_names)
    • No
      • Why?
    • n/a
  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents)

@trentm trentm self-assigned this Apr 21, 2022
@trentm trentm requested a review from felixbarny April 21, 2022 18:43
@apmmachine
Copy link

apmmachine commented Apr 21, 2022

💚 Build Succeeded

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: 2022-05-02T04:01:00.079+0000

  • Duration: 3 min 1 sec

Copy link
Contributor

@jaggederest jaggederest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also happen in the case of e.g. a delayed promise that starts a span, right? I think this is logically fine. Edit: On behalf of APM Agent Ruby though my permissions may not be 100% correct to get github to automate it 👍

@trentm
Copy link
Member Author

trentm commented Apr 25, 2022

This could also happen in the case of e.g. a delayed promise that starts a span, right?

Yes. Or any async task like a setTimeout, as well.

@trentm trentm marked this pull request as ready for review April 25, 2022 18:04
@trentm trentm requested review from a team as code owners April 25, 2022 18:04
@trentm trentm removed the request for review from a team April 25, 2022 18:05
@trentm trentm merged commit fc0a725 into main May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants