getsentry / sentry

Developer-first error tracking and performance monitoring
https://sentry.io
Other
37.54k stars 4.04k forks source link

Replay associated error timestamp #71890

Open vaind opened 1 month ago

vaind commented 1 month ago

Environment

SaaS (https://sentry.io/)

Steps to Reproduce

I've noticed this when working on RN replay touch breadcrumbs:

When an error is trigerred by a buton press, the error shows up before the actual touch event even though it should be the other way around. The issue is caused by the error not having an exact sub-second timestmap. However, it seems the timestmap is set correctly (with millisecond precision) when the event is sent from the javascript SDK, but then is trimmed to second-precision.

Expected Result

Breadcrumbs list touch events before the error caused by the touch event.

Actual Result

Example (the last listed touch event is the one that caused the error):

image

Product Area

Unknown

Link

https://sentry-sdks.sentry.io/replays/7eaabdb7c8374de292e9dbb841d9b777/?project=5428561&query=&referrer=%2Freplays%2F&statsPeriod=24h&t_main=breadcrumbs&yAxis=count%28%29

DSN

No response

Version

No response

getsantry[bot] commented 1 month ago

Assigning to @getsentry/support for routing ⏲️

getsantry[bot] commented 1 month ago

Routing to @getsentry/product-owners-replays for triage ⏲️

cmanallen commented 1 month ago

@vaind I think its a data-type issue. Type 3 is an int and type 5 is a float.

JS SDK:

Screenshot 2024-06-03 at 9 05 29 AM

RN SDK:

Screenshot 2024-06-03 at 9 55 32 AM
vaind commented 1 month ago

I think its a data-type issue. Type 3 is an int and type 5 is a float.

Where exactly are you seeing these?

cmanallen commented 1 month ago

@vaind In the network tab. The request ending in /recording-segments/ shows RN SDK type 5 events with a integer timestamp. I assume there's some issue with the way the client parses those ints that's causing your ordering issues. But I'm not sure.

cc @ryan953 @michellewzhang

michellewzhang commented 1 month ago

cc @billyvg

michellewzhang commented 1 month ago

@vaind where are you seeing the events being trimmed to second-precision?

vaind commented 1 month ago

@vaind where are you seeing the events being trimmed to second-precision?

In the error JSON as shown in the Issues section (https://us.sentry.io/api/0/projects/sentry-sdks/sentry-react-native/events/998a79f1f18341b7b9d5747ff38f650e/json/). But it may not be relevant to ordering in replay, considering what @cmanallen pointed out.

ryan953 commented 1 month ago

It looks like the timestamp is being truncated to second-precision. The endpoint to check is /replays-events-meta/ and not /events/

Look at this example from the linked replay: https://us.sentry.io/api/0/organizations/sentry-sdks/replays-events-meta/?cursor=0%3A0%3A0&dataset=discover&end=2024-05-31T10%3A52%3A42.000Z&per_page=50&project=-1&query=replayId%3A%5B7eaabdb7c8374de292e9dbb841d9b777%5D&start=2024-05-31T10%3A52%3A04.000Z

The payload contains: timestamp: "2024-05-31T10:52:20+00:00",

It's coming from here somehow: https://github.com/getsentry/sentry/blob/6e7d168da43c6b2bd0a9ad6139457843dc6fa024/src/sentry/replays/endpoints/organization_replay_events_meta.py#L71-L81

What needs to happen is we need to serialize the timestamp into a unixtimestamp, it should be a number in either second-precision or ms-precision like timestamp: 1717152740.687, or timestamp: 1717152740687. ms-precision would be much better and would not require a corresponding update on the UI side.

cc @aliu3ntry can you take a look and put a fix up for us? 🙏

cmanallen commented 1 month ago

@ryan953 Why is this just a problem for the RN SDK and not the Javascript SDK?

billyvg commented 1 month ago

@cmanallen it's been a problem for JS SDK FWIW, but it's possible it's an SDK problem there.

aliu39 commented 3 weeks ago

@ryan953

The endpoint to check is /replays-events-meta/ and not /events/

From what I looked at, it seems like the replay endpoint queries Discover under the hood, same as /events. It doesn't do anything to format/truncate the timestamp field.

From the examples in sdk docs, it looks like we support milliseconds, but it's up to the SDK to send them.

vaind commented 3 weeks ago

AFAICT the timestmap has the millisecond precision when the event is sent from the SDK.

cmanallen commented 3 weeks ago

Timestamps are stored to second precision. There's nothing we can do on the back-end. The web client will need to special case this to fix it.

cc @ryan953 @vaind

vaind commented 2 weeks ago

To me the other of breadcrumbs is pretty important because it implies causality, i.e. that the error was caused by the click. As it is, the error seem to happen before user action, which is incorrect.

What can be done to make this work?

bruno-garcia commented 2 weeks ago

Lets follow up sometime next week once @aliu39 is back from pto

aliu39 commented 2 weeks ago

The team will be looking into ways to fill in the missing precision this week