getsentry / sentry-javascript

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

Improve Redux State Context presentation when attachReduxState option is used or when context is too large #9220

Open Fwang36 opened 12 months ago

Fwang36 commented 12 months ago

Problem Statement

Feedback from a user.

Currently, when a state is trimmed due to size limits we do not visually signal the user that the context has been trimmed. This is especially confusing when the user also uses the attachReduxState option, where now a portion of the state is in the middle of the issue and the full state at the bottom. The user scrolling through may not realize that the context is trimmed.

Solution Brainstorm

We should attach something to the context to signify that the context is being trimmed similarly to how we do it for messages with the ...

Maybe we should remove that context altogether when attachReduxState is used.

AleshaOleg commented 8 months ago

@Fwang36 can you please elaborate? I tried now with the massive Redux state (2000 items for one of the slices of Redux store) and can see that state trimmed after 1000 items with [MaxProperties ~] property value, instead of real value.

However, from the screenshot below you can see that all of the properties going in alphabetic order. And that means that sometimes the real last value wouldn't be displayed at the very bottom, and even can be somewhere in the middle.

Am I getting the issue correctly?

Screenshot from 2024-01-24 01-12-21

Fwang36 commented 8 months ago

Hey @AleshaOleg ,

Thanks for taking a look!

From what I remember of the user's original issue (I'm not fully sure if I am remembering everything correctly), the redux state gets attached as a Context to the event. That makes it limited by our size limits on context objects of 8kb. We work around this by providing the attachReduxState option in the sdk. This option makes it so that the Redux State gets attached to the event as an attachment all the way to the bottom.

The result of this is 2 sets of the Redux state in the event. One as a context object, and one as a attachment. There was no indication that the context object is getting trimmed by the context size limits, so users may assume they are seeing the whole state without ever checking the attachment.

My suggestion would be maybe for the sdk to not create the context object at all, when attachReduxState is enabled.

lforst commented 8 months ago

It would definitely make sense to have better control over whether the state is attached as context or as attachment. There are considerations with quota for attachments so disabling the context object when attachReduxState is enabled is not the best approach imo

AleshaOleg commented 8 months ago

Sorry guys, I was confused. On the screenshot I added above I was showing context (and I thought it's an attachment). I was having a problem seeing attachments for the Redux state.

And after debugging I found out that attachReduxState doesn't work anymore, so I created PR to fix it - https://github.com/getsentry/sentry-javascript/pull/10381.

Meanwhile, I'll think about how it would be better to solve the issue by showing context/attachment for this issue. Will keep you updated.

AleshaOleg commented 8 months ago

What are you thinking about this solution? Inserting !!! key - property pair to an object that exceeds the maximum limit?

I chose !!! key, by default, because ! is the first symbol in the case of alphabetic sorting. So !!! key always will pop up at the very top of the Redux state object, as objects aren't keeping positions of keys, and by default, it seems it's sorted alphabetically.

Now there's a warning that exists too, but I think the user simply doesn't see it in most of the cases, as currently warning attaches to a real key name and it might pop up somewhere in the middle of this object, and obviously user will not see it. And using !!! should solve this problem.

As @lforst mentioned there's a limit for attachments, so I didn't explain to the user that instead user needs to use attachReduxState parameter and check attachments as it might also not always work for him. So I think just a gentle warning [MaxProperties ~] would work.

Screenshot from 2024-02-08 02-34-59

lforst commented 8 months ago

Yeah something like that could work. I am still not entirely sold on the !!!. Ideally we build something in the product that allows us to show warnings for particular contexts.

AleshaOleg commented 5 months ago

@lforst I made some changes in API, and now sending "...":"[MaxProperties ~]" at the bottom of the Redux state object. But on the client side, it still shows on top of the Application state. So I need to tweak the client a bit.

The question is. Is there any way to run a client locally to modify it? I didn't find any info.

Regarding the Attachment tab, I'll think later about how it can be done better

Screenshot from 2024-05-04 21-50-41

Screenshot from 2024-05-04 21-53-59

lforst commented 5 months ago

Is there any way to run a client locally to modify it?

@AleshaOleg Can you elaborate what you mean by this?

AleshaOleg commented 5 months ago

@lforst yes, I want to change the logic of how users can see State (Redux), therefore I want to know where I can find code for the web client (on screenshot). And maybe there's a possibility to run dev env locally, with dev server and so on? Just didn't find any related repository here - https://github.com/orgs/getsentry/repositories.

Screenshot from 2024-05-08 22-29-53

lforst commented 5 months ago

@AleshaOleg Oh, I see! Yes, the repository for this is https://github.com/getsentry/sentry but you probably want to follow the guide on how to set up the dev environment: https://develop.sentry.dev/environment/

Sadly, it is very involved and I hate setting it up personally but I am sure you can pull through 😂

You probably want to sync with the issues team though for this change - what I want to avoid is you putting a lot of work into a PR just for us to reject it. @rachrwang is the person for this I believe.

leeandher commented 4 months ago

Hi @AleshaOleg, thanks for the suggested solution!

We actually have a system for displaying metadata like size limitations but it may not be getting applied correctly in this case. We've also recently reworked context on the client side so it might be a good time to take a look at this. Would you be able to provide a link to a test event, or perhaps just the event payload for one of these issues with a large redux state?

AleshaOleg commented 4 months ago

Hi @leeandher! I have this one for example - https://aleshaoleg.sentry.io/issues/5232136729/?environment=production&project=4506622892834816&query=is%3Aunresolved&referrer=issue-stream&statsPeriod=30d&stream_index=0

I see now that there's no Application State -> State (Redux) field. But Attachments for Redux state are much more easier to navigate.

So, I don't think issue now makes sense, in my opinion it can be closed.