Skip to content

Commit

Permalink
[RUM-291]-SDK: send browser logs even when cookies are disabled - set…
Browse files Browse the repository at this point in the history
… a different RUM and LOGS valid browsing context / do not pass a sessionId when the cookies are off
  • Loading branch information
mquentin committed Feb 6, 2020
1 parent ecff387 commit 1f35d65
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 21 deletions.
4 changes: 3 additions & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
export { DEFAULT_CONFIGURATION, Configuration, UserConfiguration } from './configuration'
export { ErrorMessage, ErrorContext, HttpContext, ErrorOrigin, ErrorObservable } from './errorCollection'
export { BuildEnv, Datacenter, Environment, makeStub, makeGlobal, commonInit, isValidBrowsingContext } from './init'
export {
BrowsingContext, BuildEnv, Datacenter, Environment, makeStub, makeGlobal, commonInit, isValidBrowsingContext,
} from './init'
export { MonitoringMessage, monitored, monitor, addMonitoringMessage } from './internalMonitoring'
export { Observable } from './observable'
export { RequestType, RequestDetails, startRequestCollection, RequestObservable } from './requestCollection'
Expand Down
32 changes: 23 additions & 9 deletions packages/core/src/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ export function makeGlobal<T>(stub: T): T {
export type Datacenter = 'eu' | 'us'
export type Environment = 'production' | 'staging' | 'e2e-test'

export enum BrowsingContext {
RUM = 'rum',
LOGS = 'logs'
}

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

Add a new concept of BrowsingContext for each sdk package that will define the validity of the context independently of each other.

export interface BuildEnv {
datacenter: Datacenter
env: Environment
Expand All @@ -42,16 +47,25 @@ export function commonInit(userConfiguration: UserConfiguration, buildEnv: Build
}
}

export function isValidBrowsingContext() {
if (!areCookiesAuthorized()) {
console.error('Cookies are not authorized, we will not send any data.')
return false
}
if (isLocalFile()) {
console.error('Execution is not allowed in the current context.')
return false
export function isValidBrowsingContext(browsingContext: BrowsingContext) {
switch (browsingContext) {
case BrowsingContext.RUM:
if (!areCookiesAuthorized()) {
console.error('Cookies are not authorized, RUM will not send any data.')
return false
}
if (isLocalFile()) {
console.error('Execution is not allowed in the current context.')
return false
}
return true
case BrowsingContext.LOGS:
if (isLocalFile()) {
console.error('Execution is not allowed in the current context.')
return false
}

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

isValidBrowsingContext has now several modes depending of the package. Also, when the cookies are of, the SDK should not create irrelevant log (like the previous console.error('Cookies are not authorized, we will not send any data.')) as the LOGS package will generate some logs.

This comment has been minimized.

Copy link
@bcaudan

bcaudan Feb 6, 2020

Contributor

I am not in favor of this approach because:

  • core is a dependency of rum and logs, it should not be aware of its dependents
  • let's wait to have more conditions to check before building an abstraction in core

For now, let's KISS. It should be enough to expose some methods to check file and cookies and let rum and logs compose them.

return true
}
return true
}

function isLocalFile() {
Expand Down
10 changes: 6 additions & 4 deletions packages/logs/src/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import lodashMerge from 'lodash.merge'

import { LoggerSession } from './loggerSession'
import { LogsGlobal } from './logs.entry'
import { areCookiesAuthorized } from '../../core/src/cookie'

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

I need to know if the session_id is relevant or not: see comment below


export enum StatusType {
debug = 'debug',
Expand All @@ -34,6 +35,7 @@ export const STATUSES = Object.keys(StatusType)
export interface LogsMessage {
message: string
status: StatusType

[key: string]: ContextValue
}

Expand Down Expand Up @@ -63,15 +65,15 @@ export function startLogger(errorObservable: ErrorObservable, configuration: Con
lodashMerge(
{
date: new Date().getTime(),
session_id: session.getId(),
view: {
referrer: document.referrer,
url: window.location.href,
},
},
(areCookiesAuthorized()) ? { session_id: session.getId() } : {},

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

What is the easiest way of knowing if the session_id is relevant or not (cookies=off means that there are random and reloaded for each page)

This comment has been minimized.

Copy link
@bcaudan

bcaudan Feb 6, 2020

Contributor

What about making session.getId() return undefined in that case?
It should remove session_id attribute.

globalContext,
getRUMInternalContext()
) as Context
getRUMInternalContext(),
) as Context,
)
const handlers = {
[HandlerType.console]: (message: LogsMessage) => console.log(`${message.status}: ${message.message}`),
Expand Down Expand Up @@ -119,7 +121,7 @@ export class Logger {
private handlers: { [key in HandlerType]: (message: LogsMessage) => void },
handler = HandlerType.http,
private level = StatusType.debug,
private loggerContext: Context = {}
private loggerContext: Context = {},
) {
this.handler = this.handlers[handler]
}
Expand Down
16 changes: 12 additions & 4 deletions packages/logs/src/loggerSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,26 @@ export function startLoggerSession(configuration: Configuration): LoggerSession

return {
getId: session.getId,
isTracked: () => session.getType() === LoggerSessionType.TRACKED,
isTracked: () => (session.getType()) ? session.getType() === LoggerSessionType.TRACKED : computeSessionType(configuration) === LoggerSessionType.TRACKED,

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

if no cookies available, there is no type (default value is undefined), compute the Type on the fly to see if we track the log or not

This comment has been minimized.

Copy link
@bcaudan

bcaudan Feb 6, 2020

Contributor

This strategy seems a bit confusing to me.

Since we can't track session when we don't have cookies, what about trying something where we don't call startLoggerSession?

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

We still need to implement the this.session.isTracked() method that can be 0 or 1 according to the configuration sampleRate. If we don't call startLoggerSession we will have to duplicate the process of 'randomly' tracking or not the page according to this value.

This comment has been minimized.

Copy link
@bcaudan

bcaudan Feb 6, 2020

Contributor

fair point, it could be nice to add some unit tests on loggerSession to cover these cases and document the intent.

Since loggerSession need to behaves differently regarding of cookies, what about passing this information to startLoggerSession and reflect that in the implementation?
It would be easier to understand what we want to achieve here.

}
}

function computeSessionType(configuration: Configuration): string {
let sessionType
if (!performDraw(configuration.sampleRate)) {
sessionType = LoggerSessionType.NOT_TRACKED
} else {
sessionType = LoggerSessionType.TRACKED
}
return sessionType
}

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

need this function to calculate on the fly the TRACKED or not type of the session in the case of cookies=OFF

function computeSessionState(configuration: Configuration, rawSessionType?: string) {
let sessionType
if (hasValidLoggerSession(rawSessionType)) {
sessionType = rawSessionType
} else if (!performDraw(configuration.sampleRate)) {
sessionType = LoggerSessionType.NOT_TRACKED
} else {
sessionType = LoggerSessionType.TRACKED
sessionType = computeSessionType(configuration)
}
return {
isTracked: sessionType === LoggerSessionType.TRACKED,
Expand Down
3 changes: 2 additions & 1 deletion packages/logs/src/logs.entry.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
BrowsingContext,
commonInit,
Context,
ContextValue,
Expand Down Expand Up @@ -77,7 +78,7 @@ export type LogsGlobal = typeof STUBBED_LOGS
export const datadogLogs = makeGlobal(STUBBED_LOGS)
let isAlreadyInitialized = false
datadogLogs.init = monitor((userConfiguration: LogsUserConfiguration) => {
if (!isValidBrowsingContext() || !canInitLogs(userConfiguration)) {
if (!isValidBrowsingContext(BrowsingContext.LOGS) || !canInitLogs(userConfiguration)) {
return
}

Expand Down
5 changes: 3 additions & 2 deletions packages/rum/src/rum.entry.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
BrowsingContext,
commonInit,
Context,
ContextValue,
Expand Down Expand Up @@ -54,7 +55,7 @@ export type RumGlobal = typeof STUBBED_RUM
export const datadogRum = makeGlobal(STUBBED_RUM)
let isAlreadyInitialized = false
datadogRum.init = monitor((userConfiguration: RumUserConfiguration) => {
if (!isValidBrowsingContext() || !canInitRum(userConfiguration)) {
if (!isValidBrowsingContext(BrowsingContext.RUM) || !canInitRum(userConfiguration)) {

This comment has been minimized.

Copy link
@mquentin

mquentin Feb 6, 2020

Author Member

set the BrowsingContext which is specific to RUM

return
}
if (userConfiguration.publicApiKey) {
Expand All @@ -70,7 +71,7 @@ datadogRum.init = monitor((userConfiguration: RumUserConfiguration) => {

errorObservable.subscribe((errorMessage) => lifeCycle.notify(LifeCycleEventType.ERROR_COLLECTED, errorMessage))
requestObservable.subscribe((requestDetails) =>
lifeCycle.notify(LifeCycleEventType.REQUEST_COLLECTED, requestDetails)
lifeCycle.notify(LifeCycleEventType.REQUEST_COLLECTED, requestDetails),
)

const globalApi = startRum(rumUserConfiguration.applicationId, lifeCycle, configuration, session)
Expand Down

0 comments on commit 1f35d65

Please sign in to comment.