chore: add telemetry to the proxy #26695
Conversation
…try using TS conventions
| @@ -541,6 +541,7 @@ export class CommandQueue extends Queue<$Command> { | |||
| current.fail() | |||
| } | |||
|
|
|||
| Cypress.action('cy:command:failed', current, err) | |||
There was a problem hiding this comment.
double checking - Should this be command:fails OR command:skipped dep on the command state of queued vs pending?
this would be primarily observers around cy.session spans and error handling if I remember corretly.
There was a problem hiding this comment.
I don't expect skipped to count as an error. There is an explicit skipped:command:end message called. Even so, the span would be ended with the state logged, but it would also include the error.
|
|
||
| function getRandomColorFn () { | ||
| return chalk.hex(`#${Number( | ||
| Math.floor(Math.random() * 0xFFFFFF), | ||
| ).toString(16).padStart(6, 'F').toUpperCase()}`) | ||
| } | ||
|
|
||
| export const isVerboseTelemetry = true |
There was a problem hiding this comment.
Why is this exported and why is this hard-coded to true?
There was a problem hiding this comment.
It's a single place to control the verbose telemetry flag for the proxy, the request and response middle-wares both import it.
|
|
||
| const types = ['child', 'root'] as const | ||
| const SERVICE_NAME = 'cypress-app' |
There was a problem hiding this comment.
| const SERVICE_NAME = 'cypress-app' | |
| const STAGING_ENV = 'cypress-app-staging' | |
| const PRODUCTION_ENG = 'cypress-app' |
There was a problem hiding this comment.
The service name remains the same regardless of staging or prod, we flex cloud endpoint we send data too to separate staging and production.
There was a problem hiding this comment.
oops yeah, this wasn't meant to replace the Service_name, more like should we pull this up so we can see face-up the names/envs we push data to.
| opts = {}, | ||
| key, | ||
| isVerbose = false, |
There was a problem hiding this comment.
did you consider creating a wrapper called createVerboseSpan which would remove the need to define isVerbose throughout the code?
const span = startVerboseSpan(spanOpts)
const startVerboseSpan = (opts) => {
if (!this.verbose) return undefined
return startSpan(opts)
}
There was a problem hiding this comment.
I think how we have it is fine for now, a single command with options... In the future we should enhance verbose to allow targeted span creation (the real question there is how to filter and ensure parent spans are created).
There was a problem hiding this comment.
My thought is it would remove the need to pass isVerbose throughout the code-base and need to export the isVerbose boolean
There was a problem hiding this comment.
I mean we could remove that need just by setting it in the proxy, that isVerbose export is just for filtering verbosity on an off within the proxy, not the entire code base. 🤷♂️ The better way to do that would be a better filtering mechanism like we have with debugging, but for now our cheap 'isVerbose' works well enough. (imo)
packages/telemetry/src/span-exporters/console-trace-link-exporter.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const doesTopNeedSimulation = doesTopNeedToBeSimulated(this) | ||
|
|
||
| // TODO: might not need these on the span as they are declared above and might trickle down |
There was a problem hiding this comment.
Should this TODO be addressed?
| this.res.redirect(this.config.clientRoute) | ||
|
|
||
| span?.end() | ||
|
|
||
| // TODO: where is this? Do we need to pass the span in here? |
There was a problem hiding this comment.
Should this TODO be addressed?
| this._uniqueTraces[traceId] = spanId | ||
|
|
||
| this._log( | ||
| `Trace start: [${span.name}] - ${this._traceUrl}=${span.spanContext().traceId}`, |
There was a problem hiding this comment.
| `Trace start: [${span.name}] - ${this._traceUrl}=${span.spanContext().traceId}`, | |
| `Trace start: [${span.name}] - ${this._traceUrl}=${span.spanContext().traceId}`, |
| @@ -0,0 +1,97 @@ | |||
| import { expect } from 'chai' | |||
There was a problem hiding this comment.
This filename has a typo in it. Should be githubActionsDetectorSync.spec.ts?
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
This PR adds telemetry spans to the proxy as well as enhancing telemetry itself by:
CYPRESS_INTERNAL_ENABLE_TELEMETRY_VERBOSECorrectly setup ts so that the telemetry package will auto build.Edit: this caused build issues. Reverted until we have more time to investigate it.Not all spans in the proxy are properly closed, the cost of chasing down these few spans started to outweigh the value of closing them so, i decided to get this pr in since it adds value in its current form.
Future consideration: It would be nice to rework verbose mode to work like our debug verbose mode to allow for fine grain tuning of what you want sent.
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?