-
Notifications
You must be signed in to change notification settings - Fork 142
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
[REPLAY] Add public function to get the link to current Replay #2047
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2047 +/- ##
==========================================
+ Coverage 93.60% 93.63% +0.02%
==========================================
Files 192 196 +4
Lines 6008 6060 +52
Branches 1349 1362 +13
==========================================
+ Hits 5624 5674 +50
- Misses 384 386 +2
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
3d7e085
to
f37d429
Compare
6ab3f76
to
9d8fb61
Compare
a27afad
to
3926e6b
Compare
3926e6b
to
ee0d030
Compare
/** | ||
* Test for Browser features used while recording | ||
*/ | ||
export function isBrowserSupported() { | ||
return ( | ||
// Array.from is a bit less supported by browsers than CSSSupportsRule, but has higher chances | ||
// to be polyfilled. Test for both to be more confident. We could add more things if we find out | ||
// this test is not sufficient. | ||
typeof Array.from === 'function' && typeof CSSSupportsRule === 'function' && 'forEach' in NodeList.prototype | ||
) | ||
} |
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.
note: extracted from recorderApi.ts
to avoid a circular dep
|
||
const link = getSessionReplayLink({ ...DEFAULT_CONFIGURATION, site, subdomain }, sessionManager, viewContexts) | ||
|
||
expect(link).toBe(`https://${host}/rum/replay/sessions/session-id-1?seed=view-id-1&from=123456`) |
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.
question: this one fails on the browser stack with IE because error-type=browser-not-supported
gets rightfully added. Should I disable the test on IE11? Should I re-use the helper isBrowserSupported
in the unit tests here? Should I mock isBrowserSupported
to force it disabled on every browser?
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.
Maybe change the expectation on IE?
expect(link).toBe(`https://${host}/rum/replay/sessions/session-id-1?seed=view-id-1&from=123456`) | |
expect(link).toBe(isIE() ? ... : `https://${host}/rum/replay/sessions/session-id-1?seed=view-id-1&from=123456`) |
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.
🤔 still breaking on chrome/android, should I use isBrowserSupported
instead of isIE
?
https://gitlab.ddbuild.io/DataDog/browser-sdk/-/jobs/249222844
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.
@ThibautGeriz the job is failing on IE11 no?
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.
it seems I cannot read apparently
|
||
const link = getSessionReplayLink({ ...DEFAULT_CONFIGURATION, site, subdomain }, sessionManager, viewContexts) | ||
|
||
expect(link).toBe(`https://${host}/rum/replay/sessions/session-id-1?seed=view-id-1&from=123456`) |
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.
Maybe change the expectation on IE?
expect(link).toBe(`https://${host}/rum/replay/sessions/session-id-1?seed=view-id-1&from=123456`) | |
expect(link).toBe(isIE() ? ... : `https://${host}/rum/replay/sessions/session-id-1?seed=view-id-1&from=123456`) |
@@ -0,0 +1,20 @@ | |||
import type { RumConfiguration } from './configuration' | |||
|
|||
export function getDatadogOrigin(rumConfiguration: RumConfiguration) { |
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.
💬 suggestion: to be consistent with public docs naming
export function getDatadogOrigin(rumConfiguration: RumConfiguration) { | |
export function getDatadogSiteUrl(rumConfiguration: RumConfiguration) { |
return `https://${subdomain ? `${subdomain}.` : ''}${site}` | ||
} | ||
|
||
function getDefaultSubdomain(configuration: RumConfiguration): string | undefined { |
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.
💬 suggestion: for clarity + no need to pass the whole configuration
function getDefaultSubdomain(configuration: RumConfiguration): string | undefined { | |
function getSiteDefaultSubdomain(site: string): string | undefined { |
case 'datadoghq.com': | ||
return 'app' | ||
case 'datadoghq.eu': | ||
return 'app' |
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.
🥜 nitpick:
case 'datadoghq.com': | |
return 'app' | |
case 'datadoghq.eu': | |
return 'app' | |
case 'datadoghq.com': | |
case 'datadoghq.eu': | |
return 'app' |
case 'datadoghq.com': | ||
return 'app' | ||
case 'datadoghq.eu': | ||
return 'app' | ||
case 'datad0g.com': |
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.
💬 suggestion: we could use/add constants from intakeSites
export const INTAKE_SITE_STAGING = 'datad0g.com' | |
export const INTAKE_SITE_US1 = 'datadoghq.com' | |
export const INTAKE_SITE_AP1 = 'ap1.datadoghq.com' | |
export const INTAKE_SITE_US1_FED = 'ddog-gov.com' |
it(`should return ${host} for subdomain "${ | ||
subdomain ?? 'undefined' | ||
}" on "${site}" with query params if view is found`, () => { | ||
const link = getDatadogOrigin({ ...DEFAULT_CONFIGURATION, site, subdomain }) |
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.
💬 suggestion: DEFAULT_CONFIGURATION
does not seem to be used
const link = getDatadogOrigin({ ...DEFAULT_CONFIGURATION, site, subdomain }) | |
const link = getDatadogOrigin({ site, subdomain } as RumConfiguration) |
): string | undefined { | ||
const session = sessionManager.findTrackedSession() | ||
const parameters: string[] = [] | ||
const sessionId = session ? session.id : 'session-id' |
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.
💬 suggestion: what about no-session-id
for the value when there is no session id?
const origin = getDatadogOrigin(configuration) | ||
return `${origin}/rum/replay/sessions/session-id?error-type=slim-package` |
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.
💭 thought: it could be interesting to mutualise the building logic of the url with the rum
one to avoid to forget to apply future changes here
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.
something like that? Where should that go? packages/rum-core/src/domain
?
type QueryParams = { errorType?: string; seed?: string; ts?: number }
function getSessionReplayUrl(configuration: RumConfiguration, { errorType, seed, ts }: QueryParams): string {
// impl
}
we could even remove the seed
& ts
params and pass down the arg: ViewContexts
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.
packages/rum-core/src/domain
?
LGTM, you could probably have a single file for that and getDatadogOrigin
if (view && !getReplayStats(view.id)) { | ||
return 'replay-not-started' | ||
} |
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.
💬 suggestion: what about using recorder state directly rather than accessing replay stats?
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.
I will leave @BenoitZugmeyer answer this one as I lack the knowledge here
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.
It's a good idea. You could do something like this: https://gist.github.com/BenoitZugmeyer/b0c6a09367b4a7970211cb66c7eea5d3 .
site: 'datad0g.com', | ||
} as RumConfiguration | ||
|
||
describe('getReplayLink', () => { |
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.
💬 suggestion: Could you add a test for replay-not-started
?
Adding a case when replay is not started
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.
💬 suggestion: what about renaming this file to getSessionReplayUrl
?
FMU, getDatadogSiteUrl
is only exposed for tests
const sessionId = session ? session.id : 'no-session-id' | ||
const queryParams: SessionReplayUrlQueryParams = {} | ||
|
||
const errorType = getErrorType(session, isRecordingStarted) | ||
if (errorType) { | ||
queryParams.errorType = errorType | ||
} | ||
|
||
const view = viewContexts.findView() | ||
if (view) { | ||
queryParams.seed = view.id | ||
queryParams.from = view.startClocks.timeStamp | ||
} | ||
|
||
return getSessionReplayUrl(configuration, sessionId, queryParams) |
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.
💬 suggestion: what about passing directly session
, view
and errorType
to getSessionReplayUrl
?
it would avoid to declare query params outside of the link computation and would avoid to have no-session-id
declared at several spots.
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
4375726
to
36d5195
Compare
36d5195
to
0333598
Compare
Motivation
Add public function to get the link to current Replay
Changes
Add public function to get the link to current Replay
Testing
I have gone over the contributing documentation.