-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
docs(sdks): New Span API #11939
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
docs(sdks): New Span API #11939
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
| --- | ||
|
|
||
| <Alert level="info"> | ||
| This document uses key words such as "MUST", "SHOULD", and "MAY" as defined in [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt) to indicate requirement levels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mention here that this only applies to non-OTEL SDKs.
Generally, I still feel a bit conflicted about designing a new Span API that is not compatible out of the box and by design with OTEL-based SDKs, which are the most important SDKs AFAIK (JS & Python, though not sure if python can possibly do these changes anyhow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As soon as we're clear with the Performance teams roadmap, I'd like to revamp the API discussion and make sure each team proposes, what they would think would make most sense. I think this puts us in a better situation to actually discuss different solutions and understand the reasoning behind them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will stay a draft until then, so work that has been done up until now does not get lost
|
|
Updated this PR with the new API design discussed upfront with @cleptric and @Litarnus. The spec is minimal on purpose and hands a lot of additional API design decision to individual SDK authors to ensure APIs work with the platform. We can of course refine and iterate on the minim specification further. |
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can iterate on missing points (just noting them down from our last convo):
- we need an option to set a fully remote span as the parent span aka
- we need something like
sampled: booleanto force a sampling decision
| const validationSpan = Sentry.startSpan({ name: 'validate-shopping-cart'}) | ||
| startFormValidation().then((result) => { | ||
| validationSpan.setAttribute('valid-form-data', result.success); | ||
| processSpan.end(); | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The example code incorrectly manages span lifecycles, failing to end validationSpan and prematurely calling processSpan.end() before processSpan is declared.
Severity: HIGH | Confidence: 1.00
🔍 Detailed Analysis
The example code incorrectly manages span lifecycles. The validationSpan created on line 120 is never explicitly ended, leading to a resource leak. Additionally, processSpan.end() is called inside the startFormValidation().then() callback, which is logically incorrect as this callback is associated with validationSpan. Furthermore, processSpan is referenced at line 123 before its declaration on line 126, which, while technically working due to hoisting, is semantically confusing and poor practice. This example contradicts the specification's requirement for explicit span ending.
💡 Suggested Fix
Ensure validationSpan is explicitly ended after its asynchronous operation completes. Declare processSpan before it is referenced, and call processSpan.end() at the appropriate point in its lifecycle, not within the validationSpan's callback.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: develop-docs/sdk/telemetry/spans/span-api.mdx#L120-L124
Potential issue: The example code incorrectly manages span lifecycles. The
`validationSpan` created on line 120 is never explicitly ended, leading to a resource
leak. Additionally, `processSpan.end()` is called inside the
`startFormValidation().then()` callback, which is logically incorrect as this callback
is associated with `validationSpan`. Furthermore, `processSpan` is referenced at line
123 before its declaration on line 126, which, while technically working due to
hoisting, is semantically confusing and poor practice. This example contradicts the
specification's requirement for explicit span ending.
Did we get this right? 👍 / 👎 to inform future reviews.
| startFormValidation().then((result) => { | ||
| validationSpan.setAttribute('valid-form-data', result.success); | ||
| processSpan.end(); | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ntry/sentry-docs into docs/span-metrics-examples * 'docs/span-metrics-examples' of https://github.com/getsentry/sentry-docs: (29 commits) chore: Rename 404 lint job (#15403) Update prevent Discord link (#15402) fix incorrect urls (#15400) fix: Typos (#15401) feat(replay): Add screenshotStrategy option for React Native (#15334) docs(sdks): New Span API (#11939) fix (docs) Render integartions inside highlight block (#15388) fix(python): `mcp.transport` correct values (#15394) docs(limits): Update docs to match new size limits for events/attachments (#15395) Fix typos across file types & add automation (#15385) Fix 404 linter (#15312) feat(billing): Updated invoice terms to receipts and bills (#15374) fix: Update GitHub app URL from apps/sentry-io to /apps/sentry (#15386) fix(billing): Correct billing-related actions for roles (#15370) Document global attributes (#15279) docs(godot): Update before-send examples (#15306) docs(godot): User Feedback UI (aka widget) (#15304) fix(python): Remove memcached references (#15380) feat(native): external crash reporter (#15244) docs(self-hosted): provide more insights on troubleshooting kafka (#15131) ...
No description provided.