Conversation
…er performance timing is available
Contributor
size-limit report 📦
|
Lms24
approved these changes
Jan 16, 2024
Member
Lms24
left a comment
There was a problem hiding this comment.
I'm not sure I understand why Relay drops an entire transaction because of a "bad" span but guarding for this makes sense IMO.
anonrig
reviewed
Jan 16, 2024
Contributor
anonrig
left a comment
There was a problem hiding this comment.
Can we add a test so we don't regress in the future?
mydea
reviewed
Jan 17, 2024
| // It is possible that we are collecting these metrics when the page hasn't finished loading yet, for example when the HTML slowly streams in. | ||
| // In this case, ie. when the document request hasn't finished yet, `entry.responseEnd` will be 0. | ||
| // In order not to produce faulty spans, where the end timestamp is before the start timestamp, we will only collect | ||
| // these spans when the responseEnd value is available. Relay would drop the entire transaction if it contained faulty spans. |
Member
There was a problem hiding this comment.
Suggested change
| // these spans when the responseEnd value is available. Relay would drop the entire transaction if it contained faulty spans. | |
| // these spans when the responseEnd value is available. The backend would drop the entire transaction if it contained faulty spans. |
Maybe a bit more generic, for outside readers :)
mydea
reviewed
Jan 17, 2024
| // In this case, ie. when the document request hasn't finished yet, `entry.responseEnd` will be 0. | ||
| // In order not to produce faulty spans, where the end timestamp is before the start timestamp, we will only collect | ||
| // these spans when the responseEnd value is available. Relay would drop the entire transaction if it contained faulty spans. | ||
| if (entry.responseEnd !== 0) { |
Member
There was a problem hiding this comment.
Suggested change
| if (entry.responseEnd !== 0) { | |
| if (entry.responseEnd) { |
save a few bytes and should be equally correct here?
mydea
approved these changes
Jan 17, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I found that Relay was dropping pageload transactions from my Next.js test app pretty consistently so in my debugging journey I discovered that the reason was that we were sending spans with start timestamps larger than the end timestamps.
Root cause was that we assumed that the
responseEndproperty on thenavigationperformance entry is always present. It is, however, when the response hasn't finished yet, it is0.As a fallback behaviour, this PR changes the logic so we will only collect these performance entry spans when the
responseEndlogic is available.We should probably delay finishing of the pageload transaction until domContentLoded fired or the timeout is reached...