getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.97k stars 1.57k forks source link

INP not being collected in SvelteKit project #11344

Closed knd775 closed 4 months ago

knd775 commented 7 months ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/sveltekit

SDK Version

7.108.0

Framework Version

SvelteKit 2.5.4

Link to Sentry event

No response

SDK Setup

Sentry.init({
  dsn: __DSN__,
  tracesSampleRate: 1.0,
  replaysSessionSampleRate: 0.1,
  replaysOnErrorSampleRate: 1.0,
  integrations: [
    replayIntegration({ maskAllInputs: false, maskAllText: false }),
    Sentry.browserTracingIntegration({
      enableInp: true
    })
  ],
  environment: PUBLIC_SENTRY_ENV ?? 'unknown'
});

Steps to Reproduce

  1. Set enableInp option to true
  2. Visited some pages

Expected Result

INP data to be reported in the dashboard

Actual Result

No INP data (been enabled for 6 days now) image

Lms24 commented 7 months ago

UPDATE: I might be wrong; please see @edwardgou-sentry's answer below

(leaving this here only for context)


Hey @knd775 thanks for writing in!

I reproduced this with the standard sverdle app and using browserTracingIntegration({enableInp: true}). After debugging it a little I tried switching to the deprecated new BrowserTracing({enableInp: true}) class-based integration. Turns out, INP is recorded then. This makes me suspect that there is some problem in this part of the code where we try merging your custom provided browserTracing integration with the one that's set by our SvelteKit SDK by default: https://github.com/getsentry/sentry-javascript/blob/25af1509295e4db7457047f60638d4e53f537a3c/packages/sveltekit/src/client/sdk.ts/#L50-L66

As a workaround for now, would you mind replacing browserTracingIntegration with new BrowserTracing?

 integrations: [
    replayIntegration({ maskAllInputs: false, maskAllText: false }),
    new Sentry.BrowserTracing({
      enableInp: true
    })
  ],

Let me know if this fixes the issue for you, thx :)

We should definitely fix this in v7 but right now, our resources are a little tight with everyone working on the new major (v8) of the SDK. Ideally, this problem will be fixed with v8 automatically, where we drastically simplified the integration API.

edwardgou-sentry commented 7 months ago

Hey @Lms24 and @knd775 , I set up the basic sverdle app and installed @sentry/sveltekit version 7.108.0. I was able to get INP spans sent just using the basic config with no issues: integrations: [browserTracingIntegration({enableInp: true})] The INP values also surface on my Sentry org. It seems like it should be working for sveltekit without any further configuration.

@knd775 Would you be able to share a public page where you have this instrumented? or perhaps code?

michaelmairegger commented 7 months ago

Hey @Lms24 Seems that even with @sentry/angular-ivy no inp data is transferred.

Version installed 7.109.0

I have tried both: new Sentry.BrowserTracing and Sentry.browserTracingIntegration

Lms24 commented 7 months ago

@michaelmairegger if the issue you're experiencing spans across multiple SDKs, my suspicion from my initial reply is very likely incorrect - sorry about that! The apps you're testing INP with: Are they just boilerplate sample apps (like the create-svelte Sverdle app or Angular's tour of heroes default boilerplate) or are they actual projects? I'm a bit lost still where to start debugging this properly, especially because we don't have a ton of tests yet for INP in specific frameworks.

@edwardgou-sentry any ideas why this might happen? Random thought: are we discarding very short INP values? Another thing I noticed: We have a lot of early returns in the code that takes care of creating the INP spans: https://github.com/getsentry/sentry-javascript/blob/9bfadb5852312d5fac7103867436de4a5da377df/packages/tracing-internal/src/browser/metrics/index.ts/#L253-L319. Any chance we're hitting one of them consequtively for all INP values? Maybe something's off with the mapping? It might be helpful to add some debug logs around the early returns.

michaelmairegger commented 7 months ago

@Lms24 No, it is not boilerplate.

I debugged a bit and found the following:

https://github.com/getsentry/sentry-javascript/blob/9bfadb5852312d5fac7103867436de4a5da377df/packages/tracing-internal/src/browser/metrics/index.ts/#L169-L178 returns addClsInstrumentationHandler whereas https://github.com/getsentry/sentry-javascript/blob/9bfadb5852312d5fac7103867436de4a5da377df/packages/tracing-internal/src/browser/metrics/index.ts/#L157C25-L161 returns another function that calls inpCallBack(). inpCallback is function of _trackINP where https://github.com/getsentry/sentry-javascript/blob/9bfadb5852312d5fac7103867436de4a5da377df/packages/tracing-internal/src/browser/metrics/index.ts/#L257-L318 itself returns addClsInstrumentationHandler

I found out that https://github.com/getsentry/sentry-javascript/blob/9bfadb5852312d5fac7103867436de4a5da377df/packages/tracing-internal/src/browser/metrics/index.ts/#L160 is never called

michaelmairegger commented 7 months ago

@Lms24 forget my last post, I debugged more and found out that https://github.com/getsentry/sentry-javascript/blob/9bfadb5852312d5fac7103867436de4a5da377df/packages/tracing-internal/src/browser/metrics/index.ts/#L312 is called and envelope called.

Is the event not processed on backend of sentry? Does it has something todo because i use on-prem installation?

Lms24 commented 7 months ago

thanks a lot for the debugging effort! So just to confirm, you can see outgoing requests to Sentry with your INP span envelopes?

Does it has something todo because i use on-prem installation?

Oh yes this might very well be the cause! Didn't think about self-hosted because the bug report template says you're using Sentry SaaS. Looking at the self-hosted changelog, the latest version is able to ingest spans. Not sure though if self-hosted already fully supports INP. @edwardgou-sentry maybe knows :)

edwardgou-sentry commented 7 months ago

Hey @michaelmairegger , thanks for debugging! Our current self-hosted release is missing a feature flag required to process INP spans: organizations:standalone-span-ingestion It will be included by default in the next release, but try adding it manually for now and see if it works for you. Also, you will need to update to version 24.3.0 of self-hosted if you have not done so already.

michaelmairegger commented 7 months ago

@Lms24 Yes, I see outgoing INP spans as the following:

{"sent_at":"2024-04-05T15:50:24.231Z","dsn":"...."}
{"type":"span"}
{"data":{"sentry.origin":"manual","sentry.op":"ui.interaction.click","release":"...","environment":"production","transaction":"/customers","user":"..."},"description":"ul.navbar-nav.me-auto > li.nav-item > a.nav-link","op":"ui.interaction.click","span_id":"b3b9924ef06b6aee","start_timestamp":1712332212.5856001,"timestamp":1712332212.6256,"trace_id":"03a901d137214997b9877f75a3cf7145","origin":"manual","exclusive_time":40,"measurements":{"inp":{"value":40,"unit":"millisecond"}}}

The envelope is only send after following two steps have been performed:

So far I recognized only one INP envelope

I have opted into organizations:standalone-span-ingestion but so far I do not have any data in the sentry-portal. I have 24.3.0 installed. So I will wait for the next update and keep you updated

Lms24 commented 6 months ago

thx for the update, I hope the next self-hosted update fixes things.

From the SDK perspective, the INP envelope as well as the sending behaviour seems to work as expected, based on your description.

michaelmairegger commented 6 months ago

Yes, I think so. I will try after 15.04. with the new 24.4 update and report results back

michaelmairegger commented 6 months ago

@Lms24 I have now updated to 24.4 and unfortunately no INP data is shown

Note: sentry.conf.py is in sync with sentry.conf.example.py

Lms24 commented 6 months ago

Hey @michaelmairegger thanks for getting back to us and apologies for the inconvenience! I forwarded your issue to our performance team internally. My gut feeling is that something is missing in self-hosted for INP which prevents the data from being processed correctly.

Just a shot in the dark idea: (now that I think about it, this is super unlikely but I'll note it down for completeness anyway): Any chance you're filtering requests to your Sentry instance based on request payload? INP span request payloads are a bit different from regular Sentry payloads in the sense that they use a new envelope type.

michaelmairegger commented 6 months ago

@Lms24 No I have no Inbound Data Filter defined and I am not using the Relay

edwardgou-sentry commented 6 months ago

Hi @michaelmairegger , thanks for getting back! I just tested a clean install of Sentry self hosted (24.4.0) with the basic sverdle app instrumented and I was able to get INP on my self hosted instance with default configs, so we should be able to get INP working here for you.

Based on the information you provided, it looks like you're able to get INP envelopes sent, and the payload looks correct to me, so there is likely no issue with the frontend.

Are you able to pull out any logs from the Relay container on your self hosted instance? If there's an issue with consuming INPs in the backend, it's likely happening in Relay.

michaelmairegger commented 6 months ago

@edwardgou-sentry I am not using a relay service, just the docker-compose from the self-hosted github dir.

It seems that I now have INP data, but for one of my projets. So it seems that it is working

edwardgou-sentry commented 6 months ago

Thanks for the update @michaelmairegger , glad that it's working! I'll close this issue for now, but if you experience any issues again feel free to let us know!

knd775 commented 5 months ago

Hey, I was waiting for v8 of the SDK to see if the issue was resolved (it wasn't), but it looks like the issue was closed in the meantime.

Lms24 commented 5 months ago

@knd775 can you provide a minimal reproduction with the latest SDK version? We already tried reproducing this and got INP spans in a SvelteKit app.

knd775 commented 5 months ago

I'll try!

getsantry[bot] commented 4 months ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀