Conversation
Integrates @clickhouse/client version 1.11.0 into the twenty-server package. Updates yarn.lock and package.json to include and resolve the new dependency.
Introduce ClickHouse module, service, and migration framework to support analytics data storage. Includes environment variable configuration, migration files, and schema setup for events and pageview tables.
Added ClickHouse service to AnalyticsService for event storage. Events are now conditionally stored in appropriate tables, with error handling in place for failures. Introduced a new type for analytics event names to ensure type safety.
Contributor
packages/twenty-server/src/database/clickhouse/migrations/001-create-events-table.sql
Show resolved
Hide resolved
Removed support for CLICKHOUSE_USER and CLICKHOUSE_PASSWORD in configuration, service, and migrations. This simplifies ClickHouse integration by relying on default authentication mechanisms.
Removed the PARTITION BY clause from the `events` and `pageview` tables in the ClickHouse migrations. This simplifies table structure while retaining the ORDER BY configuration for efficient querying.
# Conflicts: # packages/twenty-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx # packages/twenty-front/src/pages/settings/serverless-functions/SettingsServerlessFunctionDetail.tsx # packages/twenty-server/src/engine/core-modules/analytics/analytics.service.ts # packages/twenty-server/src/engine/core-modules/environment/constants/environment-variables-group-metadata.ts
Added a newline at the end of the analytics service file to adhere to coding standards and avoid potential formatting issues. This ensures consistency across the codebase.
The analytics service was no longer being utilized and has been deleted to clean up unused code. This helps reduce maintenance overhead and improves codebase clarity.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Removed `NODE_TLS_REJECT_UNAUTHORIZED` setting to improve security and ensure proper TLS validation. This change eliminates potential security risks associated with bypassing certificate verification.
FelixMalfait
reviewed
Apr 4, 2025
|
|
||
| async insert(data: Pageview | Event) { | ||
| try { | ||
| await this.clickhouseClient.insert({ |
Member
There was a problem hiding this comment.
Is this the asyncInsert we talked about?
Contributor
Author
There was a problem hiding this comment.
Know that the async insert and the bulk insert are implemented
…ing (#11383) Introduce support for `customDomain.activated` and `customDomain.created` events with new schemas, tests, and fixtures. Refactor analytics services to enable type-safe event and pageview tracking, improve integration with ClickHouse, and enhance type definitions.
FelixMalfait
changed the base branch from
main
to
migrate-workflow-types-to-twenty-shared
# Conflicts: # packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx
Replaced USER_SIGNUP_EVENT with USER_SIGNUP_EVENT_NAME to reflect the updated constant location and naming. This ensures better consistency and alignment with the new structure in the workspace-query-runner module.
Replaced ZodSchema with ZodObject in the event registry to ensure stricter schema validation. Enhanced the registerEvent function to merge schemas with the genericTrackSchema for consistent structure.
Replaced ZodObject with ZodSchema in eventsRegistry to allow broader schema compatibility. This change ensures better flexibility for event schema definitions.
| export const TRACK = gql` | ||
| mutation Track($action: String!, $payload: JSON!) { | ||
| track(action: $action, payload: $payload) { | ||
| mutation Track( |
Member
There was a problem hiding this comment.
We have to be careful when rolling out this kind of changes that breaks the API called by the frontend, how do we make sure it doesn't trigger big errors for every person that hasn't yet refreshed their frontend? It frustrates me to spend time on this and slow us down at our stage, but on the other hand I don't see how we could circumvent it
Member
|
@greptileai review |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces ClickHouse integration for analytics by adding a ClickHouse service and updating related modules, tests, and infrastructure configuration. Key changes include:
- Integration of ClickHouse in the analytics module and updated provider registration.
- New scripts for running ClickHouse migrations and seeding.
- Updates to frontend event tracking, including GraphQL mutation and tests.
Reviewed Changes
Copilot reviewed 56 out of 63 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-server/src/engine/core-modules/analytics/analytics.module.ts | Updated module registration with ClickhouseService and revised AnalyticsService import path. |
| packages/twenty-server/src/engine/core-modules/analytics/analytics.exception.ts | Introduced a custom AnalyticsException and related error codes. |
| packages/twenty-server/src/engine/api/graphql/workspace-query-runner/listeners/telemetry.listener.ts | Updated analytics calls to use createAnalyticsContext and track functions with new event constants. |
| packages/twenty-server/src/database/clickhouse/seeds/run-seeds.ts | Added a ClickHouse seeding script using the ClickHouse client. |
| packages/twenty-server/src/database/clickhouse/migrations/run-migrations.ts | Added a ClickHouse migration script with logic to ensure the database and migration table exist. |
| packages/twenty-front/src/modules/app/effect-components/PageChangeEffect.tsx | Modified event tracking to utilize AnalyticsType and set the page title dynamically. |
| packages/twenty-front/src/modules/analytics/hooks/useEventTracker.ts | Updated hook signature to match new analytics mutation structure. |
| packages/twenty-front/src/modules/analytics/hooks/tests/useEventTracker.test.tsx | Updated tests to reflect the new analytics mutation arguments and behavior. |
| packages/twenty-front/src/modules/analytics/graphql/queries/track.ts | Updated GraphQL mutation query for tracking events. |
| packages/twenty-front/src/generated-metadata/graphql.ts | Adjusted MutationTrackArgs to the new parameter structure. |
| packages/twenty-front/.eslintrc.cjs | Removed an unused import as part of linting improvements. |
| .github/workflows/ci-server.yaml | Added a ClickHouse service container with migration and seeding steps for integration tests. |
Files not reviewed (7)
- Makefile: Language not supported
- packages/twenty-server/.env.example: Language not supported
- packages/twenty-server/.env.test: Language not supported
- packages/twenty-server/package.json: Language not supported
- packages/twenty-server/project.json: Language not supported
- packages/twenty-server/src/database/clickhouse/migrations/001-create-events-table.sql: Language not supported
- packages/twenty-server/src/database/clickhouse/migrations/002-create-pageview-table.sql: Language not supported
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR implements ClickHouse integration for analytics tracking with tables for events and pageviews, along with supporting services and utilities.
- Fixed syntax error in
/packages/twenty-server/src/database/clickhouse/migrations/002-create-pageview-table.sql- missing commas between column definitions and missing timestamp column referenced in ORDER BY - Added comprehensive analytics event tracking system with type-safe schemas using Zod in
/packages/twenty-server/src/engine/core-modules/analytics/utils/events/track - Created new
AnalyticsServiceandClickhouseServicewith proper error handling and configuration-based initialization - Implemented fluent API pattern with
createAnalyticsContext().track()for better developer experience - Added integration tests to verify events are properly stored in ClickHouse
62 file(s) reviewed, 29 comment(s)
Edit PR Review Bot Settings | Greptile
Comment on lines
+55
to
71
| query: gql` | ||
| mutation Track( | ||
| $type: AnalyticsType! | ||
| $event: String | ||
| $name: String | ||
| $properties: JSON | ||
| ) { | ||
| track( | ||
| type: $type | ||
| event: $event | ||
| name: $name | ||
| properties: $properties | ||
| ) { | ||
| success | ||
| } | ||
| } | ||
| `, |
Contributor
There was a problem hiding this comment.
style: The GraphQL query is duplicated in both mock objects. Consider extracting it to a constant to maintain DRY principles and make future updates easier.
Suggested change
| query: gql` | |
| mutation Track( | |
| $type: AnalyticsType! | |
| $event: String | |
| $name: String | |
| $properties: JSON | |
| ) { | |
| track( | |
| type: $type | |
| event: $event | |
| name: $name | |
| properties: $properties | |
| ) { | |
| success | |
| } | |
| } | |
| `, | |
| const TRACK_MUTATION = gql` | |
| mutation Track( | |
| $type: AnalyticsType! | |
| $event: String | |
| $name: String | |
| $properties: JSON | |
| ) { | |
| track( | |
| type: $type | |
| event: $event | |
| name: $name | |
| properties: $properties | |
| ) { | |
| success | |
| } | |
| } | |
| `; |
| # CLOUDFLARE_ZONE_ID= | ||
| # CLOUDFLARE_WEBHOOK_SECRET= No newline at end of file | ||
| # CLOUDFLARE_WEBHOOK_SECRET= | ||
| # ANALYTICS_ENABLED= |
Contributor
There was a problem hiding this comment.
style: Consider adding a default value like false for ANALYTICS_ENABLED to make it clear what the default behavior is when not specified.
| }; | ||
|
|
||
| async function ensureDatabaseExists() { | ||
| const [url, database] = clickhouseUrl().split(/\/(?=[^/]*$)/); |
Contributor
There was a problem hiding this comment.
style: The regex pattern splits the URL at the last forward slash to separate the database name. This could fail if the URL contains authentication credentials with slashes.
Suggested change
| const [url, database] = clickhouseUrl().split(/\/(?=[^/]*$)/); | |
| const url = new URL(clickhouseUrl()); | |
| const database = url.pathname.slice(1); | |
| const baseUrl = url.origin; |
Comment on lines
+3
to
+4
| `name` LowCardinality(String) | ||
| `properties` JSON |
Contributor
There was a problem hiding this comment.
syntax: Missing comma after the name column definition. Each column definition except the last one should be followed by a comma.
Suggested change
| `name` LowCardinality(String) | |
| `properties` JSON | |
| `name` LowCardinality(String), | |
| `properties` JSON |
Comment on lines
+4
to
+5
| `properties` JSON | ||
| `userId` String DEFAULT '', |
Contributor
There was a problem hiding this comment.
syntax: Missing comma after the properties column definition.
Suggested change
| `properties` JSON | |
| `userId` String DEFAULT '', | |
| `properties` JSON, | |
| `userId` String DEFAULT '', |
packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/workspace/__tests__/workspace.service.spec.ts
Show resolved
Hide resolved
...twenty-server/src/engine/metadata-modules/serverless-function/serverless-function.service.ts
Show resolved
Hide resolved
packages/twenty-server/src/modules/timeline/jobs/create-audit-log-from-internal-event.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/modules/timeline/jobs/create-audit-log-from-internal-event.ts
Outdated
Show resolved
Hide resolved
Implemented `trackV2` mutation to replace the deprecated `track` mutation, updating relevant GraphQL schemas, resolvers, and associated tests. Adjustments include changes to input types, migration queries, and usage across the application.
Updated unused parameters in the track mutation to include underscores, aligning with TypeScript conventions and improving code readability. This change helps clarify that these parameters are intentionally unused.
Replaced the deprecated track method with the updated trackV2 in a test case. Ensures alignment with the latest analytics resolver changes.
Replaced the generic object record event with distinct events: Object Record Created, Updated, and Deleted. Updated the fixtures and streamlined event seeding to enhance clarity and tracking precision.ification step was unnecessary as the fixtures were already in the correct format. This change simplifies the code and ensures the seeding process works as expected.
Removed a console log statement used for debugging in the ClickHouse event registration integration test. This cleans up unnecessary output and improves test readability.
Removed an obsolete comment in the ClickHouse event registration integration test. This change simplifies the code and removes unnecessary clutter.
Updated event schemas to allow additional properties using passthrough. Simplified telemetry handling logic by removing event class checks and utilizing event name suffixes for tracking actions.
# Conflicts: # packages/twenty-server/.env.example
Removed unnecessary debug log statement in the create-audit-log-from-internal-event file. This improves the code readability and avoids noise in the console output.
Updated GraphQL mutation, types, and references to use `TrackAnalytics` instead of `TrackV2` for better clarity and standardization. Adjusted corresponding tests, hooks, and integration logic to reflect the change.
FelixMalfait
approved these changes
Apr 16, 2025
Improved readability by reformatting GraphQL query arguments and hook parameters in the analytics module. This change does not alter functionality but enhances code maintainability and clarity.
Removed duplicate service in workspace test setup and updated typings to make `properties` optional. Enhanced validation for `CLICKHOUSE_URL` with stricter URL rules. These changes enhance code maintainability and reliability.
Ensure default empty objects are assigned to properties if null or undefined in pageview and track analytics inputs. This prevents potential runtime errors when accessing properties.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.