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: Pass telemetry resources from the node process to the browser #26468

Merged
merged 8 commits into from
Apr 10, 2023

Conversation

mjhenkes
Copy link
Member

  • Closes

Additional details

I noticed that resource information like 'ci.branch' were not getting applied to spans logged from the browser, this change passes resources declared in the node process up to the browser process to ensure they also get logged.

Steps to test

Check for the presence of ci.branch on spans logged from the browser in honeycomb.

How has the user experience changed?

PR Tasks

@mjhenkes mjhenkes requested review from mschile and AtofStryker April 10, 2023 14:34
packages/telemetry/test/browser.spec.ts Outdated Show resolved Hide resolved
@@ -30,6 +30,7 @@ export interface TelemetryApi {
findActiveSpan(fn: findActiveSpanOptions): Span | undefined
endActiveSpanAndChildren (span?: Span | undefined): void
getActiveContextObject (): contextObject
getResources (): {}
Copy link
Contributor

@AtofStryker AtofStryker Apr 10, 2023

Choose a reason for hiding this comment

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

is it always a key value pair of strings in this case kinda similar to the tests?

Suggested change
getResources (): {}
getResources (): {[key: string]: string}

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's always serializable, but it might be numbers too

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe getResources (): {[key: string]: string | number}?

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, i tried that it's more complex (how about booleans? how about arrays of strings?). I'm going to try to reuse an opentelmetry type.

Copy link
Contributor

Choose a reason for hiding this comment

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

worse case I feel like {[key: string]: string} is acceptable since it sounds like it could be any serializable type. Or it is always a primitive we could do something like {[key: string]: string | number | boolean | undefined | null} (assuming no symbols or bigint)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The otel type appears to be working for us.

@@ -103,7 +103,6 @@ declare global {
__RUN_MODE_SPECS__: SpecFile[]
__CYPRESS_TESTING_TYPE__: 'e2e' | 'component'
__CYPRESS_BROWSER__: Partial<Browser> & {majorVersion: string | number}
__CYPRESS_TELEMETRY__?: {context: {traceparent: string}}
Copy link
Member Author

Choose a reason for hiding this comment

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

removed duplicative typing, at least as far as i can tell we don't need it with the type defined in browsers.ts

@cypress
Copy link

cypress bot commented Apr 10, 2023

30 flaky tests on run #45399 ↗︎

0 26909 1281 0 Flakiness 30

Details:

Update packages/telemetry/src/index.ts
Project: cypress Commit: da1e217abc
Status: Passed Duration: 18:17 💡
Started: Apr 10, 2023 6:41 PM Ended: Apr 10, 2023 6:59 PM
Flakiness  create-from-component.cy.ts • 2 flaky tests • app-e2e

View Output Video

Test Artifacts
... > runs generated spec Output Screenshots Video
... > runs generated spec Output Screenshots Video
Flakiness  runs.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
App: Runs > Runs - Create Project > when a project is created, injects new projectId into the config file, and sends expected UTM params Output Screenshots Video
Flakiness  specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs Output Screenshots Video
Flakiness  cypress-in-cypress.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
Cypress in Cypress > scales the AUT correctly in e2e Output Screenshots Video
Flakiness  cypress-origin-communicator.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
Cypress In Cypress Origin Communicator > cy.origin passivity with app interactions > passes upon test reload mid test execution Output Screenshots Video

The first 5 flaky specs are shown, see all 17 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@@ -51,8 +53,10 @@ export class OTLPTraceExporter extends OTLPTraceExporterHttp {
return
}

// Continue to send this header for passivity until the cloud is released.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue logged to remove this one the cloud is updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I was just planning to do it myself once i was notified that cloud had released.

@mjhenkes mjhenkes merged commit b4622ee into develop Apr 10, 2023
@mjhenkes mjhenkes deleted the matth/chore/pass-resources-to-browser branch April 10, 2023 19:33
astone123 pushed a commit to kgroat/cypress that referenced this pull request Apr 19, 2023
…ypress-io#26468)

* chore: pass resources from the node process to the browser

* don't check version

* Apply suggestions from code review

Co-authored-by: Bill Glesias <[email protected]>

* reuse type for attributes

* use auth header

* ts?

* Update packages/telemetry/src/index.ts

Co-authored-by: Matt Schile <[email protected]>

---------

Co-authored-by: Bill Glesias <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
tgriesser added a commit that referenced this pull request Apr 25, 2023
* feat/protocol: (45 commits)
  chore: adding support for url:changed (#26519)
  chore: adding viewport:changed to protocol (#26508)
  chore: Reduce dependencies and binary size, add circle ci detector (#26522)
  chore: 12.10.0 release (#26517)
  test: fix flaky tests (#26505)
  chore: Check project dependencies for CT compatibility (#26497)
  chore: update vm2 to 3.9.16 (#26489)
  chore: enable builds on feat/protocol branch (#26506)
  chore: [skip ci] update to labels looked at by stalebot (#26496)
  chore: connecting to electron browser (#26471)
  chore: [skip ci] turning on stale bot (#26488)
  chore: fix issue with logs without wallClockUpdatedAt (#26473)
  Update triage_add_to_project.yml
  chore: Update Chrome (stable) to 112.0.5615.49 and Chrome (beta) to 113.0.5672.24 (#26434)
  feat: display framework definition errors (#26183)
  fix: correctly resolve dependencies for CT onboarding when using Yarn Plug n Play (#26452)
  fix: Subscribe to framework detection changes in wizard (#26437)
  fix: make clicks on type('{enter}') composed (#26395)
  chore: update add-to-project workflow (#26439)
  chore: Pass telemetry resources from the node process to the browser (#26468)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants