fix(performance): Display thresholds in all summary tabs #27256
Conversation
Display the threshold and metric in all the transaction summary tabs. Perviously it only showed up in the main summary page.
| }); | ||
| }) | ||
| .catch(err => { | ||
| this.setState({loadingThreshold: false}); |
There was a problem hiding this comment.
I think this may intended behaviour, but I will ask anyways lol. Do we still want to set loadingThreshold to false even if both threshold requests fail? This would allow the user to still click the button and they would probably be able to set their threshold if nothing else breaks.
There was a problem hiding this comment.
The pattern I see in our codebase is to set the loading state to false. And it makes sense that it's no longer loading now that the API request has failed. Although you do bring up a good point about allowing users to interact with the modal if the request fails. I think it should be okay because the API endpoint will only allow them to do things they are allowed to do. I'll think about it some more.
There was a problem hiding this comment.
I found a case where the get request could fail ahha- if the user has project-transaction-threshold-override flag on but project-transaction-threshold off and they don't have a transaction threshold configured and it tries to fall back to the project threshold. I have this draft PR that would fix it, but it's kinda gross. I'm leaning towards a careful EA rollout of project-transaction-threshold-override and once we are at 100% just fully replacing project-transaction-threshold with project-transaction-threshold-override everywhere. @Zylphrex maybe you have some input on this as you do a second pass?
There was a problem hiding this comment.
If I'm understanding this correctly, this shouldn't an issue at all because the set of users with access to project-transaction-threshold-override should always be a subset of the users with access to project-transaction-threshold. And once this is in GA, this constraint will no longer be a concern.
If that's the case, I wouldn't worry too much about this, and just make sure the roll out is correct.
A scenario where this may be problematic is for on-prem where users can turn on whichever flag they want, but this is an edge case that I don't think we should think too hard about as the problem goes away once this is in GA.
| '[data-test-id="grid-editable"] [data-test-id="empty-state"]', timeout=2 | ||
| ) | ||
| # We have to wait for this again because there are loaders inside of the table | ||
| self.page.wait_until_loaded() |
There was a problem hiding this comment.
Are these additional waits no longer necessary? If not can we remove the corresponding comment?
There was a problem hiding this comment.
this one was an accidental extra wait, and the ones below are no longer necessary since opening the modal doesn't have any async stuff-- will remove 👍
There was a problem hiding this comment.
Oh I think this one I don't care about since I don't care about the stuff in the table loading. Although maybe I should add it back to prevent flakey tests
| }); | ||
| }) | ||
| .catch(err => { | ||
| this.setState({loadingThreshold: false}); |
There was a problem hiding this comment.
If I'm understanding this correctly, this shouldn't an issue at all because the set of users with access to project-transaction-threshold-override should always be a subset of the users with access to project-transaction-threshold. And once this is in GA, this constraint will no longer be a concern.
If that's the case, I wouldn't worry too much about this, and just make sure the roll out is correct.
A scenario where this may be problematic is for on-prem where users can turn on whichever flag they want, but this is an edge case that I don't think we should think too hard about as the problem goes away once this is in GA.
| if (defined(onChangeThreshold)) { | ||
| onChangeThreshold(threshold, metric); | ||
| } |
There was a problem hiding this comment.
Nit: I personally like to use optional chaining, not a blocker if you leave this alone
| if (defined(onChangeThreshold)) { | |
| onChangeThreshold(threshold, metric); | |
| } | |
| onChangeThreshold?.(threshold, metric); |
The Transaction Threshold modal was not displaying
the threshold and metric when opened from the
tags and web vitals page. This is because the api
request to retrieve this info was only being fired off
in the txn summary overview page. Updated this by moving
the api calls closer to the settings button in -
TransactionThresholdButton that lives in the txn
summary headers so the api call is made on all the pages.