-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Connect EventTracker to TB endpoint #7240
Conversation
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.
PR Summary
This PR implements a new analytics system using TinyBird, replacing the previous Cloudflare-based solution. The changes focus on improving data collection and separating cloud analytics from self-hosted telemetry.
- Added
sessionId
to analytics tracking in both frontend and backend - Introduced
ANALYTICS_ENABLED
andTINYBIRD_TOKEN
environment variables - Updated
AnalyticsService
to use TinyBird's API for event tracking - Removed
telemetry
field fromClientConfig
and related components - Modified
PageChangeEffect
to collect more detailed event data
22 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
userAgent: window.navigator.userAgent, | ||
href: window.location.href, | ||
referrer: document.referrer, | ||
timeZone: Intl.DateTimeFormat().resolvedOptions().timeZone, | ||
}); | ||
}, 500); |
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.
style: Consider removing or explaining the 500ms delay
export const setSessionId = (cookieName: string, domain?: string): void => { | ||
const sessionId = getSessionId(cookieName) || crypto.randomUUID(); | ||
const baseCookie = `${cookieName}=${sessionId}; Max-Age=1800; path=/; secure`; | ||
const cookie = domain ? baseCookie + `; domain=${domain}` : baseCookie; | ||
|
||
document.cookie = cookie; | ||
}; |
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.
logic: Ensure cookie is set with appropriate security flags (HttpOnly, SameSite) to mitigate XSS and CSRF risks
mutation Track($type: String!, $sessionId: String!, $data: JSON!) { | ||
track(type: $type, sessionId: $sessionId, data: $data) { |
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.
logic: Track mutation updated to include sessionId, but test data not provided
if (!this.environmentService.get('ANALYTICS_ENABLED')) { | ||
return { success: true }; | ||
} |
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.
style: Consider combining ANALYTICS_ENABLED and TELEMETRY_ENABLED checks to simplify logic
packages/twenty-server/src/modules/messaging/monitoring/services/messaging-telemetry.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/effect-components/PageChangeEffect.tsx
Outdated
Show resolved
Hide resolved
(eventType: string, sessionId: string, eventData: EventData) => { | ||
createEventMutation({ | ||
variables: { | ||
type: eventType, |
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.
why not copy the payload from the example? Here we send pageview event from the frontend but you'll want to track other types of event
packages/twenty-server/src/engine/core-modules/analytics/analytics.module.ts
Outdated
Show resolved
Hide resolved
version: '1', | ||
session_id: createEventInput.sessionId, | ||
payload: { | ||
userId: userId, |
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 data within the payload is in camelCase (which make sense, it's what we usually use) then you should adapt your tinybird data model to also be in snakeCase (sessionId not session_id)
@@ -25,6 +25,7 @@ export class TelemetryListener { | |||
data: { |
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.
remove type: track and pass action
instead with payload.name
as its value? eventName isn't part of your schema is it?
@@ -48,6 +49,7 @@ export class TelemetryListener { | |||
data: { | |||
eventName: 'user.signup', |
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.
should this be passed as action?
@@ -48,6 +49,7 @@ export class TelemetryListener { | |||
data: { | |||
eventName: 'user.signup', | |||
}, | |||
sessionId: '', |
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.
We could maybe create a @Session decorator to get the session during signup (from the cookie) and then pass it down to the backend events, the same way we pass userId / workspaceId?
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.
We can push this to a later PR
data: object; | ||
}; | ||
|
||
@Injectable() | ||
export class AnalyticsService { | ||
private readonly logger = new Logger(AnalyticsService.name); | ||
private readonly datasource = 'analytics_events'; |
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 also switch everything to camelCase and singular? That's our current convention in our Postgres database
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.
Also I see in Tinybird a table with the singular name has already been created, is this used for testing? you can create branch environments for testing if you need to
534b37c
to
9023a2f
Compare
Thanks @anamarn for your contribution! |
#7091
EventTrackers send information of events to the TinyBird instance:
In order to test:
Example of payload when user is logged in