-
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
Changes from 3 commits
5419e98
0346742
ee0d030
ab1c25e
c9f32ce
3e197d7
2ee287d
34c4f73
06ffd5c
31edb21
daf32b6
274d88b
837c3dd
6b5d4ac
4d5e6cd
4f0704d
4aa65e9
78975b3
58c4d9d
bd634b3
c6cb2dc
0ff7004
77e94c4
caac7ad
0333598
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import type { RumConfiguration } from '@datadog/browser-rum-core' | ||
import { getDatadogOrigin } from './getDatadogOrigin' | ||
|
||
const DEFAULT_CONFIGURATION = { | ||
site: 'datad0g.com', | ||
} as RumConfiguration | ||
|
||
describe('getDatadogOrigin', () => { | ||
const parameters: Array<[string, string | undefined, string]> = [ | ||
['datadoghq.com', undefined, 'app.datadoghq.com'], | ||
['datadoghq.com', 'toto', 'toto.datadoghq.com'], | ||
['datad0g.com', undefined, 'dd.datad0g.com'], | ||
['datad0g.com', 'toto', 'toto.datad0g.com'], | ||
['us3.datadoghq.com', undefined, 'us3.datadoghq.com'], | ||
['us3.datadoghq.com', 'toto', 'toto.us3.datadoghq.com'], | ||
['us5.datadoghq.com', undefined, 'us5.datadoghq.com'], | ||
['us5.datadoghq.com', 'toto', 'toto.us5.datadoghq.com'], | ||
] | ||
|
||
parameters.forEach(([site, subdomain, host]) => { | ||
it(`should return ${host} for subdomain "${ | ||
subdomain ?? 'undefined' | ||
}" on "${site}" with query params if view is found`, () => { | ||
const link = getDatadogOrigin({ ...DEFAULT_CONFIGURATION, site, subdomain }) | ||
|
||
expect(link).toBe(`https://${host}`) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: to be consistent with public docs naming
Suggested change
|
||||||||||||||||
const site = rumConfiguration.site | ||||||||||||||||
const subdomain = rumConfiguration.subdomain || getDefaultSubdomain(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 commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: for clarity + no need to pass the whole configuration
Suggested change
|
||||||||||||||||
switch (configuration.site) { | ||||||||||||||||
case 'datadoghq.com': | ||||||||||||||||
return 'app' | ||||||||||||||||
case 'datadoghq.eu': | ||||||||||||||||
return 'app' | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🥜 nitpick:
Suggested change
|
||||||||||||||||
case 'datad0g.com': | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: we could use/add constants from
|
||||||||||||||||
return 'dd' | ||||||||||||||||
default: | ||||||||||||||||
return undefined | ||||||||||||||||
} | ||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import type { RumConfiguration } from '@datadog/browser-rum-core' | ||
import { getSessionReplayLink } from './getSessionReplayLink' | ||
|
||
const DEFAULT_CONFIGURATION = { | ||
site: 'datad0g.com', | ||
} as RumConfiguration | ||
|
||
describe('getReplayLink (slim package)', () => { | ||
const parameters: Array<[string, string | undefined, string]> = [ | ||
['datadoghq.com', undefined, 'app.datadoghq.com'], | ||
['datadoghq.com', 'toto', 'toto.datadoghq.com'], | ||
['datad0g.com', undefined, 'dd.datad0g.com'], | ||
['datad0g.com', 'toto', 'toto.datad0g.com'], | ||
['us3.datadoghq.com', undefined, 'us3.datadoghq.com'], | ||
['us3.datadoghq.com', 'toto', 'toto.us3.datadoghq.com'], | ||
['us5.datadoghq.com', undefined, 'us5.datadoghq.com'], | ||
['us5.datadoghq.com', 'toto', 'toto.us5.datadoghq.com'], | ||
] | ||
|
||
parameters.forEach(([site, subdomain, host]) => { | ||
it(`should return ${host} for subdomain "${ | ||
subdomain ?? 'undefined' | ||
}" on "${site}" with query params if view is found`, () => { | ||
const link = getSessionReplayLink({ ...DEFAULT_CONFIGURATION, site, subdomain }) | ||
|
||
expect(link).toBe(`https://${host}/rum/replay/sessions/session-id?error-type=slim-package`) | ||
}) | ||
}) | ||
}) | ||
ThibautGeriz marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import type { RumConfiguration } from '@datadog/browser-rum-core' | ||
import { getDatadogOrigin } from '@datadog/browser-rum-core' | ||
|
||
export function getSessionReplayLink(configuration: RumConfiguration): string | undefined { | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something like that? Where should that go? type QueryParams = { errorType?: string; seed?: string; ts?: number }
function getSessionReplayUrl(configuration: RumConfiguration, { errorType, seed, ts }: QueryParams): string {
// impl
} we could even remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
LGTM, you could probably have a single file for that and |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* 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 commentThe reason will be displayed to describe this comment to others. Learn more. note: extracted from |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,83 @@ | ||||||
import type { RumConfiguration, ViewContexts } from '@datadog/browser-rum-core' | ||||||
import { createRumSessionManagerMock } from '../../../rum-core/test' | ||||||
import { getSessionReplayLink } from './getSessionReplayLink' | ||||||
import { addRecord, resetReplayStats } from './replayStats' | ||||||
|
||||||
const DEFAULT_CONFIGURATION = { | ||||||
site: 'datad0g.com', | ||||||
} as RumConfiguration | ||||||
|
||||||
describe('getReplayLink', () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion: Could you add a test for |
||||||
afterEach(() => { | ||||||
resetReplayStats() | ||||||
}) | ||||||
it('should return url without query param if no view', () => { | ||||||
const sessionManager = createRumSessionManagerMock().setId('session-id-1') | ||||||
const viewContexts = { findView: () => undefined } as ViewContexts | ||||||
|
||||||
const link = getSessionReplayLink(DEFAULT_CONFIGURATION, sessionManager, viewContexts) | ||||||
|
||||||
expect(link).toBe('https://dd.datad0g.com/rum/replay/sessions/session-id-1') | ||||||
}) | ||||||
|
||||||
const parameters: Array<[string, string | undefined, string]> = [ | ||||||
['datadoghq.com', undefined, 'app.datadoghq.com'], | ||||||
['datadoghq.com', 'toto', 'toto.datadoghq.com'], | ||||||
['datad0g.com', undefined, 'dd.datad0g.com'], | ||||||
['datad0g.com', 'toto', 'toto.datad0g.com'], | ||||||
['us3.datadoghq.com', undefined, 'us3.datadoghq.com'], | ||||||
['us3.datadoghq.com', 'toto', 'toto.us3.datadoghq.com'], | ||||||
['us5.datadoghq.com', undefined, 'us5.datadoghq.com'], | ||||||
['us5.datadoghq.com', 'toto', 'toto.us5.datadoghq.com'], | ||||||
] | ||||||
|
||||||
ThibautGeriz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
parameters.forEach(([site, subdomain, host]) => { | ||||||
it(`should return ${host} for subdomain "${ | ||||||
subdomain ?? 'undefined' | ||||||
}" on "${site}" with query params if view is found`, () => { | ||||||
const sessionManager = createRumSessionManagerMock().setId('session-id-1') | ||||||
const viewContexts = { | ||||||
findView: () => ({ | ||||||
id: 'view-id-1', | ||||||
startClocks: { | ||||||
timeStamp: 123456, | ||||||
}, | ||||||
}), | ||||||
} as ViewContexts | ||||||
addRecord('view-id-1') | ||||||
|
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. question: this one fails on the browser stack with IE because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe change the expectation on IE?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 still breaking on chrome/android, should I use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. it seems I cannot read apparently |
||||||
}) | ||||||
}) | ||||||
|
||||||
it('return a param if replay is sampled out', () => { | ||||||
const sessionManager = createRumSessionManagerMock().setId('session-id-1').setPlanWithoutSessionReplay() | ||||||
const viewContexts = { | ||||||
findView: () => ({ | ||||||
id: 'view-id-1', | ||||||
startClocks: { | ||||||
timeStamp: 123456, | ||||||
}, | ||||||
}), | ||||||
} as ViewContexts | ||||||
|
||||||
const link = getSessionReplayLink({ ...DEFAULT_CONFIGURATION, site: 'datadoghq.com' }, sessionManager, viewContexts) | ||||||
|
||||||
expect(link).toBe( | ||||||
'https://app.datadoghq.com/rum/replay/sessions/session-id-1?error-type=incorrect-session-plan&seed=view-id-1&from=123456' | ||||||
) | ||||||
}) | ||||||
|
||||||
it('return a param if rum is sampled out', () => { | ||||||
const sessionManager = createRumSessionManagerMock().setNotTracked() | ||||||
const viewContexts = { | ||||||
findView: () => undefined, | ||||||
} as ViewContexts | ||||||
|
||||||
const link = getSessionReplayLink({ ...DEFAULT_CONFIGURATION, site: 'datadoghq.com' }, sessionManager, viewContexts) | ||||||
|
||||||
expect(link).toBe('https://app.datadoghq.com/rum/replay/sessions/session-id?error-type=rum-not-tracked') | ||||||
}) | ||||||
}) |
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