-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(browser): Add INP support for v8 #11650
Conversation
size-limit report 📦
|
packages/types/src/span.ts
Outdated
@@ -54,6 +55,9 @@ export interface SpanJSON { | |||
trace_id: string; | |||
origin?: SpanOrigin; | |||
_metrics_summary?: Record<string, Array<MetricSummary>>; | |||
profile_id?: string; | |||
exclusive_time?: number; | |||
measurements: Measurements; |
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.
measurements: Measurements; | |
measurements?: Measurements; |
@@ -54,6 +55,9 @@ export interface SpanJSON { | |||
trace_id: string; | |||
origin?: SpanOrigin; | |||
_metrics_summary?: Record<string, Array<MetricSummary>>; | |||
profile_id?: string; |
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.
@jjbayer just to check if this is technically correct: Can we send these fields for a span
both in the (current) span envelope, as well as for a span in a transaction event?
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.
To be clear we wont actually set this ever for spans in a transaction event, but the types for these are shared right now...
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.
Can we send these fields for a span both in the (current) span envelope, as well as for a span in a transaction event?
Yes.
@@ -54,6 +55,9 @@ export interface SpanJSON { | |||
trace_id: string; | |||
origin?: SpanOrigin; | |||
_metrics_summary?: Record<string, Array<MetricSummary>>; | |||
profile_id?: string; |
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.
Can we send these fields for a span both in the (current) span envelope, as well as for a span in a transaction event?
Yes.
measurements: { inp: { value: expect.any(Number), unit: expect.any(String) } }, | ||
}; | ||
|
||
expect(spanEnvelopeItem[0].type).toBe('span'); |
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.
@cleptric I wonder, should we send an otel_span
here instead? Since we discussed yesterday that SDKs should not longer send span
items going forward.
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.
INP
and the upcoming HTTP.CLIENT
spans are the only exception, which only applies to the JS SDK. I'm afraid we would also delay the v8 release too much, so I think it's the pragmatic solution to move forward here.
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 eventually update this and send something else! This is all internal stuff, so we can change it in the v8 cycle I think.
await page.locator('[data-test-id=interaction-button]').click(); | ||
const envelope = await getMultipleSentryEnvelopeRequests<Event>(page, 1); | ||
const envelope = await getMultipleSentryEnvelopeRequests<SentryEvent>(page, 1); |
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.
To avoid flakes I'd recommend
- start listening for the envelope by calling getMultipleSentryEnvelopeRequests and storing the promise
- clicking
- awaiting the promise.
@@ -143,7 +143,7 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { | |||
transactionEvent.spans = spans; | |||
|
|||
const measurements = timedEventsToMeasurements(span.events); | |||
if (Object.keys(measurements).length) { | |||
if (measurements && Object.keys(measurements).length) { |
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.
I guess we can remove this check then, as we actually do this in timedEventsToMeasurements
now?
if (measurements && Object.keys(measurements).length) { | |
if (measurements) { |
@@ -281,6 +281,7 @@ function createAndFinishSpanForOtelSpan(node: SpanNode, spans: SpanJSON[], remai | |||
op, | |||
origin, | |||
_metrics_summary: getMetricSummaryJsonForSpan(span as unknown as Span), | |||
measurements: timedEventsToMeasurements(span.events), |
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.
do we actually need/want this here? 🤔
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.
I guess it's fine, lets leave it in!
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.
If there are not measurements, the key gets dropped anyway :)
packages/utils/test/envelope.test.ts
Outdated
import type { InternalGlobal } from '../src/worldwide'; | ||
import { GLOBAL_OBJ } from '../src/worldwide'; | ||
|
||
describe('envelope', () => { | ||
describe('createSpanEnvelope()', () => { | ||
it('span-envelope-item of INP event has the correct object structure', () => { | ||
const attributes: SpanAttributes = dropUndefinedKeys({ |
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.
I think we don't need dropUndefinedKeys
here as we are manually building this object!
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.
Just some small things, overall looking great!
[SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: metric.value, | ||
}); | ||
|
||
const envelope = span ? createSpanEnvelope([span]) : undefined; |
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.
span should always be defined here, no need to check for existence:
const envelope = span ? createSpanEnvelope([span]) : undefined; | |
const envelope = createSpanEnvelope([span]); |
|
||
const envelope = span ? createSpanEnvelope([span]) : undefined; | ||
const transport = client && client.getTransport(); | ||
if (transport && envelope) { |
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.
if (transport && envelope) { | |
if (transport) { |
as a sidenote: I approved the PR, even though I was working on it 😅 @mydea created the PR and could therefore not approve (but gave me the "go" to do so) |
This is a step to support INP on v8.
It tries to forward port some stuff that has been merged into v7. It is a bit tricky to do this properly as this was spread over multiple PRs and cannot be directly ported at once.
Closes #10945