size-limit report 📦

View base workflow run

maybe I'm missing something obvious but why do we need getSentryHeaders? Doesn't getTraceData already do this? I'd prefer we avoid adding another utility function if we don't strictly have to do it.

Hmm, you are right that these are kind of similar 🤔 but getTraceData uses the new getTraceHeaders under the hood, but probably we can/should unify this some more. I'll do another pass and see if/how that makes sense! 👍

❌ 15 Tests Failed:

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Nevermind, still not working - need another round 😬

@lforst & @Lms24 OK, Now for real I think it should work.

I had to change the custom getTraceData implementation in OTEL to not use the propagator directly, because this also ensured that we checked the active span's url for trace propagation targets and possibly skip the sentry headers 😬 So instead, we now call the getInjectionData method from the propagator directly in getTraceData, which should ensure we have the same implementation but without the checks.

I think this is a good refactor! A bit concerned that bundle size is increasing although it looks like we removed more code than we added 😅

yeah this is really weird 😬 not really sure where this is coming from... I think I'd still do this and hope that some follow up refactoring can safe a few bytes there. Sometimes, the ways of bundle size are not easy to understand 😬

@Lms24 so, after digging some more into why this increases bundle size, I believe it is because of this code in getTraceData:

  const isValidSentryTraceHeader = TRACEPARENT_REGEXP.test(sentryTrace);
  if (!isValidSentryTraceHeader) {
    logger.warn('Invalid sentry-trace data. Cannot generate trace data');
    return {};
  }

  const validBaggage = isValidBaggageString(baggage);
  if (!validBaggage) {
    logger.warn('Invalid baggage data. Not returning "baggage" value');
  }

which basically checks some regexes. We have not checked this before, so now we have to include these regexes in the browser bundles which increases bundle size 😬

So IMHO either we accept this, or we only check these things in debug mode (?). I am not aware of any problems we had with this so far in browser land where we did not use to check this, so I am missing context for why we are checking this so defensively here.

Honestly checking for validity of shapes is a bit weird. Our parsing/writing logic should just have sensible defaults and there is no need to be so defensive. Can we remove this checking logic?

I think it's fine to remove the validation logic!

For the record: We introduced the baggage validation logic because of XSS concerns (#9483 (comment)), originally in the Astro SDK. The Astro SDK code was later extracted to the core SDK for getTraceData.

I'm not sure though how valid the potential vector here is so chances are we were over-cautious back then.

I will merge this as is for now, and we can look into the validation in a follow up PR 👍