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

Helps users quickly understand if a resource script was loaded from a cache or not #79651

Open
Dhrumil-Sentry opened this issue Oct 23, 2024 · 3 comments
Assignees
Labels

Comments

@Dhrumil-Sentry
Copy link

Problem Statement

Lukas has made a helpful proposal here: getsentry/sentry-javascript#14056

Let's use his logic and new attribute to show users whether a resource span was fetched from a cache or not

Solution Brainstorm

.

Product Area

Explore

@Lms24
Copy link
Member

Lms24 commented Oct 24, 2024

Thanks for considering my proposal! I'll post my algorithm here again, just to have things where they belong:

function wasResourceCached(attributes: SpanAttributes): 'hit' | 'miss' | 'unknown' {
  const deliveryType = attributes['http.response_delivery_type'];
  if (deliveryType != null) {
    return deliveryType === '' ? 'miss' : 'hit';
  }

  const transferSize = attributes['http.http.response_transfer_size'] as number | undefined;

  if (!transferSize) {
    return 'unknown';
  }
  if (transferSize > 0) {
    return 'miss';
  }

  const decodedBodySize = attributes['http.decoded_response_content_length'] as number | undefined;
  if (!decodedBodySize) {
    return 'unknown';
  }

  if (decodedBodySize > 0) {
    return 'hit';
  }

  return 'unknown';
}

(There are some type issues in this code which we should adress to get rid of the typecast. Please feel free to adjust as you see fit!)

I'll also link some sources as to where I took this from:

Final remark: The first check for deliveryType diverges a bit from MDN because in my PR where I add deliveryType to the spans, I had to use "default" instead of the empty string "", which is the actual default of deliveryType. I'm still checking with ingest if there's a bug that makes us drop attributes with empty string values. If that's the case and we can fix it, I'll remove sending "default" and simply stick to the empty string to stay consistent with the browser API.

Edit: Updated algorithm to what the SDK is going to send once getsentry/sentry-javascript#13502 lands

@Lms24
Copy link
Member

Lms24 commented Oct 28, 2024

Update: I will make a slight adjustment to what specific strings we send, since the SDK depends on a Relay change (getsentry/relay#4174) to keep things better aligned with the values sent from the browser. We'll not send "default" for when a request was actually made but the empty string"", which is exactly what the browser sets in this case.

@Lms24
Copy link
Member

Lms24 commented Nov 15, 2024

Update: The SDK changes have landed in v8.37.0. So from SDK PoV, the product can now adapt the changes :)

@linear linear bot added the Migrated label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants