Skip to content
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

chore: add telemetry realworld app #26896

Merged
merged 12 commits into from
Jun 1, 2023
11 changes: 6 additions & 5 deletions .circleci/workflows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ mainBuildFilters: &mainBuildFilters
- /^release\/\d+\.\d+\.\d+$/
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- 'update-v8-snapshot-cache-on-develop'
- 'ryanm/feat/unify-cdp-approach-in-electron'
- 'matth/chore/add-telemetry-realworld-app'

# usually we don't build Mac app - it takes a long time
# but sometimes we want to really confirm we are doing the right thing
Expand Down Expand Up @@ -139,7 +139,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" && "$CIRCLE_BRANCH" != "ryanm/feat/unify-cdp-approach-in-electron" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" && "$CIRCLE_BRANCH" != "matth/chore/add-telemetry-realworld-app" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
Expand Down Expand Up @@ -752,7 +752,7 @@ commands:
command:
description: Test command to run to start Cypress tests
type: string
default: "yarn cypress:run"
default: "CYPRESS_INTERNAL_ENABLE_TELEMETRY=1 CYPRESS_RECORD_KEY=$MAIN_RECORD_KEY CYPRESS_PROJECT_ID=ypt4pf yarn cypress:run"
# if the repo to clone and test is a monorepo, you can
# run tests inside a specific subfolder
folder:
Expand Down Expand Up @@ -822,7 +822,7 @@ commands:
name: Run tests using browser "<<parameters.browser>>"
working_directory: /tmp/<<parameters.repo>>/<<parameters.folder>>
command: |
<<parameters.command>> -- --browser <<parameters.browser>>
<<parameters.command>> --browser <<parameters.browser>> --record false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was recording disabled to test this? or are you intentionally disabling?

Also not in this scope per-say, but do you know whats different between the test-binary-against-rwa and test-binary-against-repo jobs? test-binary-against-rwa seems like a copy paste?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recording was never enabled, but i needed to override the project ID and record key to send telemetry. I disabled recording to silence a warning thrown when record is not enabled.

I don't know the different between test-binary-against-repo, I didn't notice they were the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh I see, passing CYPRESS_PROJECT_ID prob caused that error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we no longer need the double -- since the command is executed with yarn?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, yarn kept yelling at me about it.

- unless:
condition: <<parameters.browser>>
steps:
Expand All @@ -839,7 +839,7 @@ commands:
- run:
name: Run tests using browser "<<parameters.browser>>"
working_directory: /tmp/<<parameters.repo>>
command: <<parameters.command>> -- --browser <<parameters.browser>>
command: <<parameters.command>> --browser <<parameters.browser>> --record false
- unless:
condition: <<parameters.browser>>
steps:
Expand Down Expand Up @@ -2767,6 +2767,7 @@ linux-x64-workflow: &linux-x64-workflow
requires:
- create-build-artifacts
- test-binary-against-cypress-realworld-app:
context: test-runner:cypress-record-key
<<: *mainBuildFilters
requires:
- create-build-artifacts
Expand Down
4 changes: 1 addition & 3 deletions packages/server/lib/cypress.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,7 @@ module.exports = {

debug('from argv %o got options %o', argv, options)

if (options.key) {
telemetry.exporter()?.attachRecordKey(options.key)
}
telemetry.exporter()?.attachRecordKey(options.key)

if (options.headless) {
// --headless is same as --headed false
Expand Down
27 changes: 27 additions & 0 deletions packages/telemetry/src/span-exporters/cloud-span-exporter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,18 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
enc: OTLPExporterNodeConfigBasePlusEncryption['encryption'] | undefined
projectId?: string
recordKey?: string
requirementsToExport: 'met'| 'unmet' | 'unknown'
sendWithHttp: typeof sendWithHttp
constructor (config: OTLPExporterNodeConfigBasePlusEncryption = {}) {
super(config)
this.enc = config.encryption
this.delayedItemsToExport = []
this.sendWithHttp = sendWithHttp
if (this.enc) {
this.requirementsToExport = 'unknown'
this.headers['x-cypress-encrypted'] = '1'
} else {
this.requirementsToExport = 'met'
}
}

Expand All @@ -50,6 +54,11 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
*/
attachProjectId (projectId: string | null | undefined): void {
if (!projectId) {
if (this.requirementsToExport === 'unknown') {
this.requirementsToExport = 'unmet'
Comment on lines +59 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need unknown and unmet? can we assume 'unmet' until met?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason this wasn't a bool?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when encryption is on, we can't assume unmet until we get a null project id or record key. The send function will behave differently. If unmet we error instantly, if unknown we will delay the request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise we could incorrectly fail any spans sent prior to getting the project id/record key, since we capture spans prior to them being available.

Copy link
Member

@emilyrohrbough emilyrohrbough Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, maybe we throw a comment in there about this? unless I missed it per the limited diff view 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

this.abortDelayedItems()
}

return
}

Expand All @@ -65,6 +74,11 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
*/
attachRecordKey (recordKey: string | null | undefined): void {
if (!recordKey) {
if (this.requirementsToExport === 'unknown') {
this.requirementsToExport = 'unmet'
this.abortDelayedItems()
}

return
}

Expand All @@ -77,6 +91,7 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
*/
setAuthorizationHeader () {
if (this.projectId && this.recordKey) {
this.requirementsToExport = 'met'
this.headers.Authorization = `Basic ${Buffer.from(`${this.projectId}:${this.recordKey}`).toString('base64')}`
this.sendDelayedItems()
}
Expand All @@ -95,6 +110,14 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
}
}

abortDelayedItems () {
this.delayedItemsToExport.forEach((item) => {
item.onError(new Error('Spans cannot be sent, exporter has unmet requirements, either project id or record key are undefined.'))
})

this.delayedItemsToExport = []
}

/**
* Overrides send if we need to encrypt the request.
* @param objects
Expand All @@ -113,6 +136,10 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
return
}

if (this.requirementsToExport === 'unmet') {
onError(new Error('Spans cannot be sent, exporter has unmet requirements, either project id or record key are undefined.'))
}

let serviceRequest: string

if (typeof objects !== 'string') {
Expand Down
72 changes: 66 additions & 6 deletions packages/telemetry/test/span-exporters/cloud-span-exporter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ describe('cloudSpanExporter', () => {
const exporter = new OTLPTraceExporter(genericRequest)

expect(exporter.headers['x-cypress-encrypted']).to.equal('1')
expect(exporter.requirementsToExport).to.equal('unknown')
expect(exporter.enc).to.not.be.undefined
})

it('does not set encrypted header if not set', () => {
const exporter = new OTLPTraceExporter()

expect(exporter.headers['x-cypress-encrypted']).to.be.undefined
expect(exporter.requirementsToExport).to.equal('met')
expect(exporter.enc).to.be.undefined
})
})
Expand All @@ -42,15 +44,20 @@ describe('cloudSpanExporter', () => {
expect(callCount).to.equal(1)
})

it('does nothing if id is not passed', () => {
const exporter = new OTLPTraceExporter()
it('sets requirements to unmet if id is not passed', () => {
const exporter = new OTLPTraceExporter(genericRequest)

let callCount = 0
let abortCallCount = 0

exporter.setAuthorizationHeader = () => {
callCount++
}

exporter.abortDelayedItems = () => {
abortCallCount++
}

expect(exporter.headers['x-project-id']).to.be.undefined
expect(exporter.projectId).to.be.undefined

Expand All @@ -59,6 +66,9 @@ describe('cloudSpanExporter', () => {
expect(exporter.headers['x-project-id']).to.be.undefined
expect(exporter.projectId).to.be.undefined
expect(callCount).to.equal(0)

expect(exporter.requirementsToExport).to.equal('unmet')
expect(abortCallCount).to.equal(1)
})
})

Expand All @@ -80,21 +90,29 @@ describe('cloudSpanExporter', () => {
expect(callCount).to.equal(1)
})

it('does nothing if record key is not passed', () => {
const exporter = new OTLPTraceExporter()
it('sets requirements to unmet if record key is not passed', () => {
const exporter = new OTLPTraceExporter(genericRequest)

let callCount = 0
let abortCallCount = 0

exporter.setAuthorizationHeader = () => {
callCount++
}

exporter.abortDelayedItems = () => {
abortCallCount++
}

expect(exporter.recordKey).to.be.undefined

exporter.attachRecordKey(undefined)

expect(exporter.recordKey).to.be.undefined
expect(callCount).to.equal(0)

expect(exporter.requirementsToExport).to.equal('unmet')
expect(abortCallCount).to.equal(1)
})
})

Expand All @@ -109,10 +127,9 @@ describe('cloudSpanExporter', () => {

const authorization = exporter.headers.Authorization

console.log('auth', authorization)

// MTIzOjQ1Ng== is 123:456 base64 encoded
expect(authorization).to.equal(`Basic MTIzOjQ1Ng==`)
expect(exporter.requirementsToExport).to.equal('met')
})
})

Expand Down Expand Up @@ -206,6 +223,24 @@ describe('cloudSpanExporter', () => {
})
})

describe('abortDelayedItems', () => {
it('aborts any delayed items', (done) => {
const exporter = new OTLPTraceExporter()

exporter.delayedItemsToExport.push({
serviceRequest: 'req',
onSuccess: () => {},
onError: (error) => {
expect(error.message).to.equal('Spans cannot be sent, exporter has unmet requirements, either project id or record key are undefined.')
done()
},
})

exporter.abortDelayedItems()
expect(exporter.delayedItemsToExport.length).to.equal(0)
})
})

describe('send', () => {
it('returns if shutdownOnce.isCalled is true', () => {
const exporter = new OTLPTraceExporter()
Expand Down Expand Up @@ -395,5 +430,30 @@ describe('cloudSpanExporter', () => {
expect(exporter.delayedItemsToExport.length).to.equal(1)
expect(exporter.delayedItemsToExport[0].serviceRequest).to.equal('string')
})

it('errors if requirements are unmet', (done) => {
const exporter = new OTLPTraceExporter()

exporter.requirementsToExport = 'unmet'

exporter.convert = (objects) => {
throw 'convert should not be called'
}

exporter.sendWithHttp = (collector, body, contentType, resolve, reject) => {
throw 'sendWithHttp should not be called'
}

const onSuccess = () => {
throw 'onSuccess should not be called'
}

const onError = (error) => {
expect(error.message).to.equal('Spans cannot be sent, exporter has unmet requirements, either project id or record key are undefined.')
done()
}

exporter.send('string', onSuccess, onError)
})
})
})