Repository navigation
Conversation
size-limit report 📦
|
f68643a to
9a5304f
Compare
msonnb
changed the title
f69e94d to
3ac6356
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
I trust your judgement and the passing tests 😆
| @@ -1,121 +1,68 @@ | |||
| sentryTest('creates a new trace for a navigation after the initial pageload', async ({ getLocalTestUrl, page }) => { | |||
There was a problem hiding this comment.
l: should be placed beneath the import statements
| shouldSkipTracingTest, | ||
| } from '../../../../utils/helpers'; | ||
|
|
||
| sentryTest('creates a new trace and sample_rand on each navigation', async ({ getLocalTestUrl, page }) => { |
There was a problem hiding this comment.
l: should be placed beneath the import statements
|
|
||
| const pageloadSpan = await pageloadSpanPromise; | ||
|
|
||
| page.goto(`${url}#foo`); |
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| integrations: [Sentry.spanStreamingIntegration()], | ||
| integrations: [], |
There was a problem hiding this comment.
super-l: we can just delete this instead of passing the empty array
| traceLifecycle: 'static', | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| integrations: [Sentry.spanStreamingIntegration()], | ||
| integrations: [], |
| `${url}#foo`, | ||
| eventAndTraceHeaderRequestParser, | ||
| ); | ||
| const [pageloadEvent, pageloadTraceHeader] = await waitForStreamedSpanAndTraceHeaderOnUrl(page, url); |
There was a problem hiding this comment.
super-l: the "event" in the variable name here doesn't make much sense anymore. We should probably just call the spans we get here pageloadSpan (and so on)
edit: looks like these naming patterns should be adusted in all the trace-lifetime tests
There was a problem hiding this comment.
m: why was this test removed? this should still be reported in static trace lifecycle mode, no?
There was a problem hiding this comment.
oops, must have slipped through when restoring the -static tests. i put it back.
| const pageloadTraceContext = pageloadEvent; | ||
| const navigationTraceContext = navigationEvent; |
There was a problem hiding this comment.
l: once renamed, we can also get rid of this, since we just assert on the span here.
| import { shouldSkipTracingTest } from '../../../utils/helpers'; | ||
| import { collectStreamedSpans, waitForStreamedSpan } from '../../../utils/spanUtils'; | ||
|
|
||
| sentryTest('streams all children without the static 1000-span limit', async ({ getLocalTestUrl, page }) => { |
There was a problem hiding this comment.
m: I think we miss the counterpart for transactions here where we should still check for the 1k limit
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
8501cd6 to
d474bba
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d474bba. Configure here.
| expect(childSpan_4_1.links).toBe(undefined); | ||
|
|
||
| expect(childSpan_4_2.description).toBe('childSpan4.2'); | ||
| expect(childSpan_4_2.name).toBe('childSpan4.2'); |
There was a problem hiding this comment.
Child span checks race envelopes
Medium Severity
linking-addLinks and linking-spanOptions only wait for root spans, then read children from collectStreamedSpans. Roots and children leave in separate envelopes, so childSpan4.1, childSpan4.2, and childSpan3.1 can still be missing, and the empty-child checks can pass before later envelopes arrive. This was flagged because the testing-conventions rule calls out this kind of multi-request race.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit d474bba. Configure here.
| expect(spans.some(span => span.description === 'take-me')).toBe(true); | ||
| expect(spans.some(span => span.description?.includes('ignore-me'))).toBe(false); | ||
| expect(spans.some(span => span.name === 'take-me')).toBe(true); | ||
| expect(spans.some(span => span.name?.includes('ignore-me'))).toBe(false); |
There was a problem hiding this comment.
Ignored spans checked before they end
Medium Severity
The streaming ignoreSpans test treats a missing ignore-me name in collectStreamedSpans as proof the span was dropped. ignore-me and ignore-me-too are still open when the shortened pageload span is emitted, so the absence check can pass before those spans end and get sent. This was flagged because the testing-conventions rule calls out waits that are not tied to the telemetry under assertion.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit d474bba. Configure here.
This branch has not been deployed
3 participants
Run the browser trace lifecycle and public span API tests with default span streaming, asserting across envelopes as roots and children are sent independently. Retain the existing transaction counterparts as explicitly pinned
*-staticsuites so both lifecycles keep regression coverage for sampling, propagation, linking, and span APIs.Cover circular attributes in both lifecycles: streamed spans drop unsupported objects while preserving primitive attributes, and static transactions retain circular-reference normalization. Filter feedback waits by envelope type so unrelated span envelopes cannot satisfy them.
Keep
public-api/startSpan/setMeasurementon the static lifecycle because the legacy measurement API has no span v2 representation.Fixes #24144