-
Notifications
You must be signed in to change notification settings - Fork 140
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
[ci-visibility] Link CI Visibility and RUM #1192
Conversation
@@ -96,9 +99,10 @@ export function startRumAssembly( | |||
date: timeStampNow(), | |||
service: configuration.service, | |||
session: { | |||
type: syntheticsContext ? SessionType.SYNTHETICS : SessionType.USER, | |||
type: syntheticsContext ? SessionType.SYNTHETICS : ciContext ? SessionType.CITEST : SessionType.USER, |
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.
You need to submit this new type in rum-event-format.
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.
Could you add a test for it?
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.
will do, thanks!
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.
PR in rum-event-format: DataDog/rum-events-format#49
Codecov Report
@@ Coverage Diff @@
## main #1192 +/- ##
==========================================
+ Coverage 89.01% 89.03% +0.01%
==========================================
Files 101 102 +1
Lines 4324 4330 +6
Branches 988 990 +2
==========================================
+ Hits 3849 3855 +6
Misses 475 475
Continue to review full report at Codecov.
|
00546e0
to
a3f4775
Compare
this should be ready for another look @bcaudan |
a couple of unit tests are failing, I'm on it |
they should be fixed |
packages/rum-core/test/specHelper.ts
Outdated
@@ -301,3 +302,19 @@ export function cleanupSyntheticsWorkerValues() { | |||
deleteCookie(SYNTHETICS_RESULT_ID_COOKIE_NAME) | |||
deleteCookie(SYNTHETICS_INJECTS_RUM_COOKIE_NAME) | |||
} | |||
|
|||
export function mockCiVisibilityWindowValues(traceId?: string | { [key: string]: string }) { |
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.
regarding the way you use it in the tests, the intent seems to be:
export function mockCiVisibilityWindowValues(traceId?: string | { [key: string]: string }) { | |
export function mockCiVisibilityWindowValues(traceId?: unknown) { |
no?
@@ -75,6 +77,7 @@ export function startRumAssembly( | |||
} | |||
|
|||
const syntheticsContext = getSyntheticsContext() | |||
const ciContext = getCiTestContext() |
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.
const ciContext = getCiTestContext() | |
const ciTestContext = getCiTestContext() |
for naming consistency
@@ -678,7 +678,7 @@ export interface CommonProperties { | |||
/** | |||
* Type of the session | |||
*/ | |||
readonly type: 'user' | 'synthetics' | |||
readonly type: 'user' | 'synthetics' | 'ci_test' |
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.
Did you run yarn rum-events-format:sync
(cf file header)?
the rum-events-format
ref should also be updated
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.
good catch
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.
Nice!
Motivation
Link RUM and CI Visibility through the cypress instrumentation.
This requires changes to the cypress instrumentation in dd-trace-js: DataDog/dd-trace-js#1706
Related event format update: DataDog/rum-events-format#49 and DataDog/rum-events-format#50
Changes
window.Cypress
, which is available under cypress tests, and pass info as context.Testing
I have gone over the contributing documentation.