-
Notifications
You must be signed in to change notification settings - Fork 65
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
feat:referrer #327
feat:referrer #327
Conversation
src/sessions/PageManager.ts
Outdated
document.referrer !== undefined && document.referrer !== '' | ||
? document.referrer | ||
: null, |
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 can extract this check&set to a separate method since we're reusing it in 3 places.
src/sessions/PageManager.ts
Outdated
document.referrer !== undefined && document.referrer !== '' | ||
? document.referrer | ||
: null, |
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: Use method mentioned in comment above
src/sessions/PageManager.ts
Outdated
document.referrer !== undefined && document.referrer !== '' | ||
? document.referrer | ||
: null, |
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: Use method mentioned in comment above
src/sessions/PageManager.ts
Outdated
'([A-Za-z]+://[^/]+)[/]?' | ||
); | ||
|
||
if (domainName) { | ||
return domainName[1]; | ||
} |
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.
questions:
- Is the purpose of the regex match to exclude trailing slashes (
/
)? - For domains, we typically exclude the
https://
prefix. Should we also exclude the prefix here as well? - Do we not want to include
localhost
in the regex match?
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.
- Yes. However, will be using URL interface instead of regex for this. (See next revision)
- Excluded in next revision
- Next revision use of URL interface should handle localhost as well
@@ -487,3 +488,86 @@ describe('PageManager tests', () => { | |||
expect(pageManager.getPage().start).toEqual(2500); | |||
}); | |||
}); | |||
|
|||
test('when complete referrer is available from the DOM', async () => { |
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: when complete referrer is available from the DOM
--> when complete referrer is available from the DOM then is recorded in page view event
); | ||
}); | ||
|
||
test('when only domain level referrer is available from the DOM', async () => { |
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: when only domain level referrer is available from the DOM
--> when only domain level referrer is available from the DOM then is recorded in page view event
); | ||
}); | ||
|
||
test('when referrer is not available from the DOM', async () => { |
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: when referrer is not available from the DOM
--> when referrer from the DOM is not valid then it is not recorded in page view event
src/sessions/PageManager.ts
Outdated
if (document.referrer !== undefined && document.referrer !== '') { | ||
const domainName = document.referrer.match( | ||
'([A-Za-z]+://[^/]+)[/]?' | ||
); | ||
|
||
if (domainName) { | ||
return domainName[1]; | ||
} | ||
} else { | ||
return null; | ||
} |
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.
if (document.referrer !== undefined && document.referrer !== '') { | |
const domainName = document.referrer.match( | |
'([A-Za-z]+://[^/]+)[/]?' | |
); | |
if (domainName) { | |
return domainName[1]; | |
} | |
} else { | |
return null; | |
} | |
try { | |
return new URL(document.referrer).hostname; | |
} catch (e) { | |
return ""; | |
} |
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.
added in next revision
src/sessions/PageManager.ts
Outdated
/* | ||
Get the referrer, if it can be read from the DOM | ||
*/ | ||
private getReferrer() { | ||
if (document.referrer !== undefined && document.referrer !== '') { | ||
return document.referrer; | ||
} else { | ||
return null; | ||
} | ||
} |
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.
issue: Do we need this helper function? document.referrer
should always be a URI or an empty 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.
fixed in next revision. The helper function is not needed
src/sessions/PageManager.ts
Outdated
if (page.referrer !== null) { | ||
pageViewEvent.referrer = page.referrer; | ||
|
||
if (page.referrerDomain !== null) { | ||
pageViewEvent.referrerDomain = page.referrerDomain; | ||
} | ||
} |
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.
if (page.referrer !== null) { | |
pageViewEvent.referrer = page.referrer; | |
if (page.referrerDomain !== null) { | |
pageViewEvent.referrerDomain = page.referrerDomain; | |
} | |
} | |
pageViewEvent.referrer = document.referrer; | |
pageViewEvent.referrerDomain = getDomainFromReferrer() |
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.
added in next revision
expect(pageManager.getAttributes()).toMatchObject({ | ||
'aws:referrer': '', | ||
'aws:referrerDomain': '' | ||
}); |
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: do we want to record in metadata if it's empty? is this a conscience decision we're making?
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.
https://developer.mozilla.org/en-US/docs/Web/API/Document/referrer
States that -The value is an empty string if the user navigated to the page directly (not through a link, but, for example, by using a bookmark).
, so we should record it
'popstate', | ||
(pageManager as any).popstateListener | ||
); | ||
}); |
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: do we need a unit test for when localhost
is the referrer?
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.
handled in next revision
@@ -1,6 +1,7 @@ | |||
import { Selector } from 'testcafe'; | |||
import { REQUEST_BODY } from '../../test-utils/integ-test-utils'; | |||
import { PAGE_VIEW_EVENT_TYPE } from '../../plugins/utils/constant'; | |||
import { create } from 'lodash'; |
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.
nit: unused
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.
removed in next revision
expect(pageManager.getAttributes()).toMatchObject({ | ||
'aws:referrer': '', | ||
'aws:referrerDomain': '' | ||
}); |
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.
https://developer.mozilla.org/en-US/docs/Web/API/Document/referrer
States that -The value is an empty string if the user navigated to the page directly (not through a link, but, for example, by using a bookmark).
, so we should record it
src/sessions/PageManager.ts
Outdated
if (document.referrer === 'localhost') { | ||
// Handle special case for localhost | ||
return 'localhost'; | ||
} | ||
return ''; |
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.
if (document.referrer === 'localhost') { | |
// Handle special case for localhost | |
return 'localhost'; | |
} | |
return ''; | |
return document.referrer === 'localhost' ? document.referrer : ''; |
private getDomainFromReferrer() { | ||
try { | ||
return new URL(document.referrer).hostname; | ||
} catch (e) { | ||
return document.referrer === 'localhost' ? document.referrer : ''; | ||
} | ||
} |
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.
nit: Consider covering these (localhost and empty referrer) with unit tests.
Currently, AWS CloudWatch RUM does not support referrer. Information about the referrer for page views is not presently available.
This change adds the ability for the RUM web client to record and dispatch referrer information. The referrer will be stored in the
referrer
field in event details of a page view event. The referrer's domain will be stored in thereferrerDomain
field in event details of a page view event.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.