[ci-visibility] Implement driver-agnostic integration with CI Visibility

[nikita-tkachenko-datadog]

Motivation

Allow CI Visibility users who use Selenium for testing their RUM-enabled pages to have their RUM sessions linked to their test executions.

Changes

• Get the ci visibility test_execution_id from cookie (it will be set by CI Visibility logic in the Datadog tracers using Selenium integration).
• Create a cookieObservable that uses the cookieStore API to be notified when the ci visibility cookie is set

Testing

• Local
• Staging
• Unit
• End to end

---

I have gone over the contributing documentation.

[bits-bot]

All committers have signed the CLA.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 98.27586% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.36%. Comparing base (282082c) to head (cbe079f).
Report is 5 commits behind head on main.

Files	Patch %	Lines
packages/rum-core/src/browser/cookieObservable.ts	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2639      +/-   ##
==========================================
+ Coverage   93.34%   93.36%   +0.02%     
==========================================
  Files         238      239       +1     
  Lines        6922     6966      +44     
  Branches     1522     1535      +13     
==========================================
+ Hits         6461     6504      +43     
- Misses        461      462       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[juan-fernandez]

LGTM. Let's see what the rum-browser team thinks :)

[juan-fernandez]

you might also need to modify packages/rum-core/test/mockCiVisibilityWindowValues.ts which is used in a couple of tests packages/rum-core/src/domain/contexts/ciTestContext.spec.ts and packages/rum-core/src/domain/assembly.spec.ts

[cit-pr-commenter]

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	155.43 KiB	156.11 KiB	689 B	+0.43%	✅
Logs	55.34 KiB	55.34 KiB	0 B	0.00%	✅
Rum Slim	101.93 KiB	102.60 KiB	689 B	+0.66%	✅
Worker	25.21 KiB	25.21 KiB	0 B	0.00%	✅

[amortemousque]

The goal is to use this observable to also watch the session cookie. (in a following PR)

[bcaudan]

❓ question: ‏are we sure that we can't have ordering issues if a single event contains multiple changes of a single cookie?

[amortemousque]

I tried with the both JS APIs but I never managed to get the same cookie twice in the same table. In fact, I got one change event for each API calls:

cookieStore.addEventListener('change', (event) => {
  if ((event.changed || []).some((c) => c.name === 'datadog-ci-visibility-test-execution-id')) {
    console.log(event.changed)
   }
})

document.cookie = 'datadog-ci-visibility-test-execution-id=foo; SameSite=None; Secure'
document.cookie = 'datadog-ci-visibility-test-execution-id=bar; SameSite=None; Secure'
document.cookie = 'datadog-ci-visibility-test-execution-id=baz; SameSite=None; Secure'

cookieStore.set({
  name: 'datadog-ci-visibility-test-execution-id',
  value: 'foo'
})

cookieStore.set({
  name: 'datadog-ci-visibility-test-execution-id',
  value: 'bar'
})

[amortemousque]

I could not find a clear answer to this question in the spec. So I asked for clarification. However I don't think it's a blocker for the current usage of the observable since the datadog-ci-visibility-test-execution-id will only be set once. Wdyt?

[bcaudan]

Sure, it could be nice to add a comment in the implementation though.

[amortemousque]

Done:

browser-sdk/packages/rum-core/src/browser/cookieObservable.ts

Lines 37 to 38 in 6141054

	// Based on our experimentation, we're assuming that entries for the same cookie cannot be in both the 'changed' and 'deleted' arrays.
	// However, due to ambiguity in the specification, we asked for clarification: https://github.com/WICG/cookie-store/issues/226

[BenoitZugmeyer]

🥜 nitpick: ‏For the sake of readability, I would rewrite this like that:

Suggested change

	const detectCookieChangeStrategy = (window as CookieStoreWindow).cookieStore
	? listenToCookieStoreChange(configuration)
	: watchCookieFallback
	return new Observable<CookieChange>((observable) =>
	detectCookieChangeStrategy(cookieName, (event) => observable.notify(event))
	)
	return new Observable<CookieChange>((observable) => {
	if ((window as CookieStoreWindow).cookieStore) {
	return listenToCookieStoreChange(configuration, cookieName, observable.notify)
	}
	return watchCookieFallback(cookieName, observable.notify)
	})

Reasoning: this removes a few indirections ('Strategy' variable, currying/intermediary function)

[amortemousque]

I was pretty much doing what you suggest, but I changed because of this comment: #2639 (comment)

[BenoitZugmeyer]

Clarity is subjective, I stand by my opinion, but feel free to dismiss 😃 This is just a nitpick

[amortemousque]

Indeed it is subjective 🙂 @bcaudan is not here to come to an agreement so let's keep it as is and revisite later if needed

[amortemousque]

/to-staging

[dd-devflow]

🚂 Branch Integration: starting soon, merge in < 0s

Commit cbe079f557 will soon be integrated into staging-14.

This build is going to start soon! (estimated merge in less than 0s)

Use /to-staging -c to cancel this operation!

[dd-devflow]

🚂 Branch Integration: This commit was successfully integrated

Commit cbe079f557 has been merged into staging-14 in merge commit 0a14c669f5.

Check out the triggered pipeline on Gitlab 🦊