✨ [RUM-14514] support session and view attribution by event startTime#36
Conversation
- move SessionManager → domain/session/, ViewCollection → domain/rum/view/ - SessionContext owns session id injection into hooks - ViewContext owns view id/name/url injection into hooks - RUM hook returns DISCARDED when session/view context is unset, telemetry returns SKIPPED
43c3d35 to
e27ec75
Compare
e27ec75 to
ead0a4a
Compare
- Swap combine order in Assembly so raw data takes precedence - Default date and context in ErrorCollection to avoid undefined overrides - Propagate startTime from error event to RawEvent for hook attribution - Add Assembly and ErrorCollection test coverage
- Enrich generateManualError with optional startTime across IPC bridge - Remove dedicated manual error button, always use electronAPI - Add context scenario proving error is attributed to correct session/view
ead0a4a to
02ddf01
Compare
cdn34dd
left a comment
There was a problem hiding this comment.
Overall looks good, just left a couple of suggestions.
|
|
||
| function assembleData<T>(rawData: unknown, hookResult: RecursivePartial<T> | undefined): T { | ||
| return (hookResult ? combine(rawData, hookResult) : rawData) as T; | ||
| return (hookResult ? combine(hookResult, rawData) : rawData) as T; |
There was a problem hiding this comment.
Is this change in priority intentional ? If so was is the reason behind it ?
There was a problem hiding this comment.
Yes, that it is the purpose of 3691760
The same order is used in browser-sdk, the idea is that the hooks can provide some data that can be overwritten by the event, as it should be more specific.
Like the global context should be overwritten by the event specific context.
Here, it was to ensure that we have the event date and not the default date provided by commonContext.
|
|
||
| export const SESSION_HISTORY_FILE_NAME = '_dd_session_history'; | ||
|
|
||
| export class SessionContext { |
There was a problem hiding this comment.
Pretty much all of the logic in both SessionContext and ViewContext is identical and since this pattern is something we could need in the future, I feel we should organize this a bit better. My suggestion would be to move all of the shared logic to an Abstract class, maybe something along these lines:
export abstract class DiskBackedContext {
private readonly history: DiskValueHistory<string>;
protected constructor(history: DiskValueHistory<string>, hooks: FormatHooks) {
this.history = history;
hooks.registerRum((params) => {
const id = this.history.find(params.startTime);
if (id === undefined) return DISCARDED;
return this.formatRum(id);
});
hooks.registerTelemetry((params) => {
const id = this.history.find(params.startTime);
if (id === undefined) return SKIPPED;
return this.formatTelemetry(id);
});
}
protected abstract formatRum(id: string): RecursivePartial<RumEvent>;
protected abstract formatTelemetry(id: string): RecursivePartial<TelemetryEvent>;
add(id: string): void {
this.history.add(id, timeStampNow());
}
close(): void {
this.history.closeActive(timeStampNow());
}
}then implement the context classes like so:
export class SessionContext extends DiskBackedContext {
private constructor(history: DiskValueHistory<string>, hooks: FormatHooks) {
super(history, hooks);
}
static async init(hooks: FormatHooks, expireDelay = SESSION_TIME_OUT_DELAY): Promise<SessionContext> {
const filePath = path.join(app.getPath('userData'), SESSION_HISTORY_FILE_NAME);
const history = await DiskValueHistory.init<string>({ filePath, expireDelay });
return new SessionContext(history, hooks);
}
protected formatRum(id: string) {
return { session: { id } };
}
protected formatTelemetry(id: string) {
return { session: { id } };
}
}There was a problem hiding this comment.
Indeed, we could factorize some things and I like the the fact that we could have a file by concept exposing the different format with less boilerplate.
However, I think we could wait a bit to see how we will handle other contexts like global, user, ...
Unlike session or view, they won't discard events if missing and I'm not sure if we should store them on disk 🤔
I'd be in favor of waiting to make progress on those to have a better understanding of what we should abstract.
Motivation
Events must be attributed to the session and view that were active at the event start time, not the current ones.
Changes
TimeStampValueHistoryandDiskValueHistoryfor temporal context lookupsSessionContextandViewContextinto dedicated domain subdirectoriesTimeStamptyping forstartTimein hooks and Assemblycombineorder so raw event attributes override hook attributesstartTimeTest instructions
See e2e scenario
context.scenario.tsChecklist