New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: improve replay upload resiliency #29174
perf: improve replay upload resiliency #29174
Conversation
a03d09b
to
766051c
Compare
766051c
to
967e876
Compare
1 flaky test on run #54695 ↗︎
Details:
cypress/e2e/commands/waiting.cy.js • 1 flaky test • 5x-driver-chrome:beta
Review all test suite changes for PR #29174 ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still trying to grok a lot of this, but the refactor / enhancements make a lot of sense and really appreciate the thorough test coverage!
packages/server/test/unit/cloud/upload/StreamActivityMonitor_spec.ts
Outdated
Show resolved
Hide resolved
packages/server/test/unit/cloud/api/putProtocolArtifact_spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Bill Glesias <bglesias@gmail.com>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
packages/server/test/unit/cloud/api/put_protocol_artifact_spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Bill Glesias <bglesias@gmail.com>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
@@ -3,6 +3,10 @@ | |||
|
|||
_Released 4/2/2024 (PENDING)_ | |||
|
|||
**Performance:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really performance? Or more stability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little of A, a little of B - but the source issue is labeled performance
packages/server/lib/cloud/api/api.ts
Outdated
@@ -9,19 +9,19 @@ const RequestErrors = require('@cypress/request-promise/errors') | |||
const { agent } = require('@packages/network') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but maybe rename this to index.ts
so that we don't have to do api/api
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 58c8daa
packages/server/test/unit/cloud/upload/stream_activity_monitor_spec.ts
Outdated
Show resolved
Hide resolved
…_spec.ts Co-authored-by: Ryan Manuel <ryanm@cypress.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look great. I love the encapsulation and it's really well documented and tested.
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Major behavioral changes:
To accomplish this:
protocol.ts
was removed in favor of usingfetch-retry-ts
, which adds retry logic on top of any provided fetch implementation. This is important, ascross-fetch
provides custom network agent support.HttpError
class, to better handle HTTP errors going forward.ProtocolManager
tocloud/api/putProtocolArtifact.ts
andcloud/upload/uploadStream
StreamActivityMonitor
that produces aTransform
stream that monitors a givenReadable
, and anAbortController
that will be signalled if the stream is inactive for too long. This is used inuploadStream.ts
to detect stalled uploads, by piping the Test Replay archive'sfs.ReadStream
through the StreamActivityMonitor'sTransform
.api.ts
tocloud/api/api.ts
to keep it colocated withputProtocolArtifact.ts
, which is too specific of an implementation for the parentcloud/
directory.There are some slight messaging changes:
Spec recording too large: db is $SIZE bytes, limit is $LIMIT bytes
has changed toSpec recording too large: artifact is $SIZE bytes, limit is $LIMIT bytes
.Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?