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

feat(webvitals): Adds INP instrumentation to browserTracingIntegration #10629

Conversation

edwardgou-sentry
Copy link
Contributor

@edwardgou-sentry edwardgou-sentry commented Feb 13, 2024

Adds an experimental enableInp flag to v8 browserTracingIntegration, which picks up INP webvital entries from the browser and sends a span envelope containing INP data.

Some notes and changes involve:

  • Adds generic span envelope types, helpers, categories, etc
  • Adds registerInpInteractionListener, which tracks a mapping of interaction ids to the origin route names. This is used to get the route name when constructing the interaction span payload. The length of the mapping is capped at 10 (therefore no creeping memory issues), similar to how getINP.ts tracks candidate interactions.
  • INP is placed in the span attributes, nested inside an measurements object. This is because the Span class doesn't have a measurements field. Not sure if this is the ideal approach, feedback appreciated here.
  • Span and envelope creation + sending happens in _trackINP
  • Should have added this to v7 (I forgot I needed this in v7), so we'll need to back port these changes.
  • Looked into added an e2e playwright test, but I ended up running into v7 vs v8 issues. I think the playwright tests are currently on v7. Plan to add these tests later.

Some TODOS for future prs:

  • Backport to v7
  • e2e tests
  • Start recording additional spans for INP once idle spans are available
  • Figure out how to extend or start profiles to capture INP events

@edwardgou-sentry edwardgou-sentry marked this pull request as ready for review February 13, 2024 15:11
@edwardgou-sentry
Copy link
Contributor Author

Resulting envelope should look something like this

{"sent_at":"2024-02-13T15:14:01.049Z"}
{"type":"span"}
{
  "data": {
    "sentry.origin": "manual",
    "sentry.op": "ui.interaction.click",
    "measurements": { "inp": { "value": 120, "unit": "millisecond" } },
    "release": "frontend@d9135c6e4543124bb23930bb8054db3a96a6069e",
    "environment": "prod"
  },
  "description": "/performance/browser/pageloads/",
  "op": "ui.interaction.click",
  "span_id": "91e77d4daa50afd8",
  "start_timestamp": 1707837238.4505,
  "timestamp": 1707837238.5705,
  "trace_id": "bb790e8e972141d99b41a8fb7ce1db34",
  "origin": "manual"
}

@edwardgou-sentry
Copy link
Contributor Author

Some context around why the changes in this PR are needed

Decision Doc on sending INP using standalone spans
https://www.notion.so/sentry/Ingesting-and-Storing-INP-8c665f20577f44268a3a06e0f4e4d1c2

Relay changes were needed to consume standalone spans including extracting INP from those spans. Here's the gh issue with problem statement:
getsentry/relay#2873
Here is the PR that was merged:
getsentry/relay#2969

For the bigger picture, we have further notes about Web Vitals + Performance Scores. I don't think this context is required, but if curious I would start here:
https://www.notion.so/sentry/Browser-Starfish-Performance-Score-30bbed2e0aa540a9a755e2bc23a124fb

@edwardgou-sentry
Copy link
Contributor Author

Closing this to break out into smaller prs and base off v7 branch:

The following 3 PRs can probably be reviewed individually:
#10702
#10704
#10706
They feature some smaller fixes and changes needed to do INP spans

And then this PR includes the actual updates to hook onto onINP and create the standalone INP span
#10709
It uses code changes from the 3 PRs above, so this can be reviewed later

And then this last PR is to add sample rate logic to the standalone INP span creation:
#10731
This can probably be reviewed last because it depends on all the other PRs

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.

1 participant