feat(browser): Add INP support for v8 by mydea · Pull Request #11650 · getsentry/sentry-javascript

mydea · 2024-04-17T10:44:53Z

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

github-actions · 2024-04-17T10:50:41Z

size-limit report 📦

Path	Size
@sentry/browser	21.67 KB (0%)
@sentry/browser (incl. Tracing)	32.64 KB (+3.94% 🔺)
@sentry/browser (incl. Tracing, Replay)	67.92 KB (+1.78% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.32 KB (+1.96% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)	71.74 KB (+1.67% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback)	84.14 KB (+1.43% 🔺)
@sentry/browser (incl. Feedback)	37.78 KB (0%)
@sentry/browser (incl. sendFeedback)	26.45 KB (0%)
@sentry/browser (incl. FeedbackAsync)	30.93 KB (0%)
@sentry/react	24.35 KB (0%)
@sentry/react (incl. Tracing)	35.54 KB (+3.6% 🔺)
@sentry/vue	25.25 KB (+0.22% 🔺)
@sentry/vue (incl. Tracing)	34.4 KB (+3.86% 🔺)
@sentry/svelte	21.79 KB (0%)
CDN Bundle	23.97 KB (0%)
CDN Bundle (incl. Tracing)	33.88 KB (+3.75% 🔺)
CDN Bundle (incl. Tracing, Replay)	67.41 KB (+1.67% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback)	83.22 KB (+1.38% 🔺)
CDN Bundle - uncompressed	70.62 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	100.67 KB (+3.5% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed	210.08 KB (+1.52% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	256.39 KB (+1.26% 🔺)
@sentry/nextjs (client)	34.77 KB (+3.36% 🔺)
@sentry/sveltekit (client)	33.13 KB (+3.86% 🔺)
@sentry/node	157.31 KB (+0.01% 🔺)

mydea · 2024-04-18T09:18:41Z

packages/types/src/span.ts

  _metrics_summary?: Record<string, Array<MetricSummary>>;
+  profile_id?: string;
+  exclusive_time?: number;
+  measurements: Measurements;


Suggested change

measurements: Measurements;

measurements?: Measurements;

mydea · 2024-04-18T09:19:30Z

packages/types/src/span.ts

  trace_id: string;
  origin?: SpanOrigin;
  _metrics_summary?: Record<string, Array<MetricSummary>>;
+  profile_id?: string;


@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?

To be clear we wont actually set this ever for spans in a transaction event, but the types for these are shared right now...

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.

jjbayer · 2024-04-18T11:37:03Z

packages/types/src/span.ts

  trace_id: string;
  origin?: SpanOrigin;
  _metrics_summary?: Record<string, Array<MetricSummary>>;
+  profile_id?: string;


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.

jjbayer · 2024-04-18T11:39:31Z

packages/utils/test/envelope.test.ts

+        measurements: { inp: { value: expect.any(Number), unit: expect.any(String) } },
+      };
+
+      expect(spanEnvelopeItem[0].type).toBe('span');


@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.

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.

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.

Lms24 · 2024-04-19T10:45:55Z

...ages/browser-integration-tests/suites/tracing/browserTracingIntegration/interactions/test.ts

      await page.locator('[data-test-id=interaction-button]').click();
-      const envelope = await getMultipleSentryEnvelopeRequests<Event>(page, 1);
+      const envelope = await getMultipleSentryEnvelopeRequests<SentryEvent>(page, 1);


To avoid flakes I'd recommend

start listening for the envelope by calling getMultipleSentryEnvelopeRequests and storing the promise

clicking

awaiting the promise.

mydea · 2024-04-22T09:35:52Z

packages/opentelemetry/src/spanExporter.ts


    const measurements = timedEventsToMeasurements(span.events);
-    if (Object.keys(measurements).length) {
+    if (measurements && Object.keys(measurements).length) {


I guess we can remove this check then, as we actually do this in timedEventsToMeasurements now?

Suggested change

if (measurements && Object.keys(measurements).length) {

if (measurements) {

mydea · 2024-04-22T09:36:15Z

packages/opentelemetry/src/spanExporter.ts

    op,
    origin,
    _metrics_summary: getMetricSummaryJsonForSpan(span as unknown as Span),
+    measurements: timedEventsToMeasurements(span.events),


do we actually need/want this here? 🤔

I guess it's fine, lets leave it in!

If there are not measurements, the key gets dropped anyway :)

mydea · 2024-04-22T09:39:57Z

packages/utils/test/envelope.test.ts

 describe('envelope', () => {
+  describe('createSpanEnvelope()', () => {
+    it('span-envelope-item of INP event has the correct object structure', () => {
+      const attributes: SpanAttributes = dropUndefinedKeys({


I think we don't need dropUndefinedKeys here as we are manually building this object!

mydea

Just some small things, overall looking great!

mydea · 2024-04-22T09:41:26Z

packages/browser-utils/src/metrics/inp.ts

+      [SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE]: metric.value,
+    });
+
+    const envelope = span ? createSpanEnvelope([span]) : undefined;


span should always be defined here, no need to check for existence:

Suggested change

const envelope = span ? createSpanEnvelope([span]) : undefined;

const envelope = createSpanEnvelope([span]);

mydea · 2024-04-22T09:41:34Z

packages/browser-utils/src/metrics/inp.ts

+
+    const envelope = span ? createSpanEnvelope([span]) : undefined;
+    const transport = client && client.getTransport();
+    if (transport && envelope) {


Suggested change

if (transport && envelope) {

if (transport) {

s1gr1d · 2024-04-22T13:31:55Z

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)

mydea assigned mydea and s1gr1d Apr 17, 2024

mydea commented Apr 18, 2024

View reviewed changes

s1gr1d force-pushed the fn/inp-v8-port branch from 7a5ab9c to f06b8ea Compare April 18, 2024 11:16

jjbayer reviewed Apr 18, 2024

View reviewed changes

Lms24 mentioned this pull request Apr 19, 2024

feat(core): Add options to start standalone (segment) spans via start*Span APIs #11696

Merged

Lms24 reviewed Apr 19, 2024

View reviewed changes

mydea and others added 8 commits April 19, 2024 14:30

WIP INP in v8??

9c41e52

add measurements to spanJSON

1811e5d

make measurements undefined

6f6cf53

try fix test

7b00a3a

add logs

f0a6094

try tests in CI - new INP tests

eab0354

try fix tests

e1ffe52

await envelopePromise after clicking

5202e76

s1gr1d force-pushed the fn/inp-v8-port branch from 1981cec to 5202e76 Compare April 19, 2024 12:30

s1gr1d and others added 3 commits April 19, 2024 16:04

fix flaky tests

7a5636b

Merge branch 'develop' into fn/inp-v8-port

e9616c0

update size limits

527e21b

s1gr1d marked this pull request as ready for review April 22, 2024 09:28

mydea commented Apr 22, 2024

View reviewed changes

review changes

4d67c6e

s1gr1d enabled auto-merge (squash) April 22, 2024 11:58

Merge branch 'develop' into fn/inp-v8-port

22718e8

s1gr1d approved these changes Apr 22, 2024

View reviewed changes

s1gr1d merged commit d679e7c into develop Apr 22, 2024

s1gr1d deleted the fn/inp-v8-port branch April 22, 2024 13:28

	if (measurements && Object.keys(measurements).length) {
	if (measurements) {

	const envelope = span ? createSpanEnvelope([span]) : undefined;
	const envelope = createSpanEnvelope([span]);

Uh oh!

Conversation

mydea commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1gr1d commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mydea commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading

s1gr1d commented Apr 22, 2024 •

edited

Loading