search

getsentry / sentry-javascript

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

Index lcp.element as a tag #13411

Closed sdzhong closed 4 weeks ago

sdzhong commented 2 months ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/browser

SDK Version

8

Framework Version

No response

Link to Sentry event

No response

Reproduction Example/SDK Setup

Please see https://demo.sentry.io/performance/summary/tags/?project=5808623&query=&statsPeriod=14d&tagKey=backendType&transaction=%2Fproducts:

Image

Steps to Reproduce

https://github.com/user-attachments/assets/bdb15e9a-3629-449c-bd33-0588d3b2b596

Expected Result

lcp.element and other lcp.* tags appear for the transaction

Actual Result

Image

sdzhong commented 2 months ago

From internal conversations:

It’s because of this in v8 https://github.com/getsentry/sentry-javascript/blob/615c670cfe283e77132339c3d9751060f30d3956/packages/browser-utils/src/metrics/browserMetrics.ts#L614

lcp element is set as an attribute (span data) instead of a tag, so it’s not being indexed automatically in the transaction any more.

Lms24 commented 2 months ago

Backlogging to triage after hackweek

Lms24 commented 2 months ago

lcp element is set as an attribute (span data) instead of a tag, so it’s not being indexed automatically in the transaction any more.

Yes, that's correct. I diffed v7 and v8 payloads for the former LCP tags and this is the difference for LCP in the transaction event:

v7:

{
  "contexts": {
    "trace": {
      "tags": {
        "lcp.element": "body > div#app > div > h1",
        "lcp.size": 24827
      }
    }
  },
  "tags": {
    "lcp.element": "body > div#app > div > h1",
    "lcp.size": 24827
  },
  "measurements": {
    "lcp": { "value": 106.20000000298023, "unit": "millisecond" }
  }
}

v8:

{
  "contexts": {
    "trace": {
      "data": {
        "lcp.element": "body > div#app > div > h1",
        "lcp.size": 24827
      }
    }
  },
  "measurements": {
    "lcp": { "value": 146.20000000298023, "unit": "millisecond" }
  }
}

Why was this changed?

In v8, we removed the span.setTag API because OpenTelemetry has no concept of tags and we needed to stay consistent with span APIs in browser and in Node (where we switchted to OpenTelemetry). So therefore, what previously was set as span.setTag was converted to getCurrentScope.setTag or span.setAttribute. In case of web vital tags, we always converted to span.setAttribute because this would ensure that the values are only applied to the pageload span and not to other events (like errors). Values set via span.setAttribute end up in the event.trace.context.data field, as shown in the example payload above.

Now the question is, how do we fix it? We could special-case this in the SDK but it increases complexity and arguably bings us closer to tags again which we generally don't want to (also for span-only streaming). So I'm wondering if we can adjust the tag extraction logic in Relay. @edwardgou-sentry do you know if this would be possible in Relay?

edwardgou-sentry commented 2 months ago

@edwardgou-sentry do you know if this would be possible in Relay?

@Lms24 If we want to extract the lcp.element from the trace context and into tags, I think it should be possible. We should be able to do something similar to this: https://github.com/getsentry/relay/blob/master/relay-event-normalization/src/event.rs#L688

I'm unsure if there would be potential issues with doing this though. Do we know if there are other tags we're missing from this change aside from the lcp ones?

Lms24 commented 2 months ago

Here's a list of values we previously set as tags on the transaction and now as attributes on the root span in the browserTracingIntegration:

I'm not sure which of these data points actually need to be extracted to tags. I was under the impression that we audited these and checked with the performance team. However, I didn't find the issue I had in mind, so I'm not too sure anymore.

Anyway, I discussed this with the team and we'd prefer the necessary values being extracted in Relay. Re-introducing attribute->tag conversion logic in the SDK (which will be intransparent for users) seems wrong, given the span only future where tags on spans will not exist anymore.

edwardgou-sentry commented 2 months ago

I think we can go with extracting lcp.element in Relay then. @bcoe has already added this ticket to the Insights board, so we can prioritize it and get someone to pick it up.

Lms24 commented 2 months ago

Perfect, thanks! I'm gonna set this as done for the Web SDK Frontend board and unassign myself from the issue given it's not fixed in SDK code.

Also, should we transfer this issue to the relay repo?

edwardgou-sentry commented 1 month ago

@getsentry/ingest dropping this in the Ingest board for now since it seems like there's work to be done in Relay to support this.

Quick summary: JS SDK sends certain tags differently between v7 and v8 payloads. Specifically lcp.element and lcp.size gets sent under the trace context now: https://github.com/getsentry/sentry-javascript/issues/13411#issuecomment-2309753477

Would it be possible to update relay to extract these tags from the new v8 structure instead?

jjbayer commented 1 month ago

@edwardgou-sentry yes if customers actually expect this field to show up as a tag, we can do this. Since you already identified a possible code location for the fix, would you be willing to implement this?

edwardgou-sentry commented 1 month ago

@edwardgou-sentry yes if customers actually expect this field to show up as a tag, we can do this. Since you already identified a possible code location for the fix, would you be willing to implement this?

@jjbayer Sure, I have some time to look at this today! Will drop a pr for review

jjbayer commented 4 weeks ago

Implemented in https://github.com/getsentry/relay/pull/4074.