feat: clickhouse container collecting api metrics#2122
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
WalkthroughWalkthroughThe changes involve the integration of a new ClickHouse package and enhancements to the analytics functionality within the API and dashboard applications. Key updates include the addition of new methods for data insertion into ClickHouse tables, the introduction of a logging feature in the dashboard, and the establishment of a new client class for database interactions. Additionally, new environment variables have been introduced to configure the ClickHouse connection for both applications. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range and nitpick comments (10)
apps/dashboard/app/(app)/logs/page.tsx (3)
6-7: Consider increasing the revalidation time.A
revalidatevalue of 0 means the page will be regenerated on every request. This might impact performance negatively.Consider increasing the value to a higher number of seconds to strike a balance between data freshness and performance, unless you specifically require the page to be regenerated on every request.
19-20: Consider making the logs limit configurable.The
limitfor retrieving logs is hardcoded to 10. This might not be optimal for all use cases.Consider making the limit configurable, either through a query parameter or a user setting, to allow for flexibility in the number of logs displayed.
21-28: Consider rendering the logs in a more user-friendly format.The logs are currently rendered as a stringified JSON, which might not be the most user-friendly format for the end-users.
Consider rendering the logs in a tabular format or using a library like
react-json-viewto provide a collapsible and expandable view of the JSON data. This would enhance the readability and usability of the logs page.Taskfile.yml (1)
25-25: Consider using secrets for storing the database credentials.The addition of the username and password to the ClickHouse connection string is necessary for authentication. The change will allow the migration task to connect to the ClickHouse database successfully.
To enhance security, consider storing the database credentials as secrets instead of hardcoding them in the configuration file. You can use environment variables or a secrets management system to inject the credentials at runtime.
For example, you can update the configuration to use environment variables:
-GOOSE_DBSTRING: "tcp://default:password@127.0.0.1:9000" +GOOSE_DBSTRING: "tcp://{{ .CLICKHOUSE_USER }}:{{ .CLICKHOUSE_PASSWORD }}@127.0.0.1:9000"Then, set the
CLICKHOUSE_USERandCLICKHOUSE_PASSWORDenvironment variables securely in your deployment environment.apps/dashboard/lib/clickhouse/index.ts (1)
6-52: Consider adding error handling and logging.To improve the diagnostics and maintainability of the code, consider adding:
- A try-catch block to handle any errors that may occur during the query execution.
- Logging statements to record the query parameters and any errors encountered.
deployment/docker-compose.yaml (2)
58-58: LGTM!Adding a password to the ClickHouse connection URL is a good security practice.
Consider storing the password securely using secrets management, such as Kubernetes Secrets or Vault, instead of directly in the compose file.
77-77: LGTM!Setting an admin password for ClickHouse is a good security practice.
Consider storing the password securely using secrets management, such as Kubernetes Secrets or Vault, instead of directly in the compose file.
internal/clickhouse-zod/src/client.ts (1)
66-74: Remove unnecessary check for 'req.schema' since it's always defined.The
schemaproperty inreqis required (as per line 60), so the checkif (req.schema)is unnecessary and can be removed to simplify the code.Apply this diff to streamline the code:
-let validatedEvents: z.output<TSchema> | z.output<TSchema>[] | undefined = undefined; -if (req.schema) { - const v = Array.isArray(events) - ? req.schema.array().safeParse(events) - : req.schema.safeParse(events); - if (!v.success) { - throw new Error(v.error.message); - } - validatedEvents = v.data; -} +const v = Array.isArray(events) + ? req.schema.array().safeParse(events) + : req.schema.safeParse(events); +if (!v.success) { + throw new Error(v.error.message); +} +const validatedEvents = v.data;apps/api/src/pkg/middleware/metrics.ts (2)
104-104: Limit the size of the logged request bodyLogging large request bodies can impact performance and may inadvertently expose sensitive data. Consider limiting the size of the logged request body.
Apply this diff to truncate the request body if it exceeds a maximum length:
request_body: requestBody, +if (requestBody.length > MAX_LOG_SIZE) { + requestBody = requestBody.substring(0, MAX_LOG_SIZE) + '...<truncated>'; +}Define
MAX_LOG_SIZEat the beginning of your file:const MAX_LOG_SIZE = 1024; // Adjust the size as appropriate
107-107: Limit the size of the logged response bodySimilar to the request body, logging large response bodies can be inefficient and risky. It's advisable to limit their size.
Apply this diff to truncate the response body:
- response_body: await c.res.clone().text(), + let responseBody = await c.res.clone().text(); + if (responseBody.length > MAX_LOG_SIZE) { + responseBody = responseBody.substring(0, MAX_LOG_SIZE) + '...<truncated>'; + } + response_body: responseBody,
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (22)
- Taskfile.yml (1 hunks)
- apps/agent/pkg/clickhouse/schema/001_create_requests_table.sql (1 hunks)
- apps/agent/pkg/clickhouse/schema/002_create_key_verifications_table.sql (2 hunks)
- apps/api/package.json (1 hunks)
- apps/api/src/pkg/analytics.ts (4 hunks)
- apps/api/src/pkg/env.ts (1 hunks)
- apps/api/src/pkg/hono/env.ts (1 hunks)
- apps/api/src/pkg/keys/service.ts (1 hunks)
- apps/api/src/pkg/middleware/init.ts (3 hunks)
- apps/api/src/pkg/middleware/metrics.ts (2 hunks)
- apps/api/src/routes/v1_keys_verifyKey.ratelimit_accuracy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_verifyKey.ts (1 hunks)
- apps/dashboard/app/(app)/logs/page.tsx (1 hunks)
- apps/dashboard/lib/clickhouse/index.ts (1 hunks)
- apps/dashboard/lib/env.ts (1 hunks)
- apps/dashboard/package.json (1 hunks)
- deployment/docker-compose.yaml (3 hunks)
- internal/clickhouse-zod/package.json (1 hunks)
- internal/clickhouse-zod/src/client.ts (1 hunks)
- internal/clickhouse-zod/src/index.ts (1 hunks)
- tools/local/src/cmd/dashboard.ts (1 hunks)
- tools/local/src/main.ts (1 hunks)
Additional comments not posted (24)
internal/clickhouse-zod/src/index.ts (1)
1-1: LGTM!The re-export statement is a common pattern used to create a single entry point for a library or module. This can make it easier for consumers to use the library, as they don't need to know the internal structure of the library.
internal/clickhouse-zod/package.json (1)
1-17: LGTM!The
package.jsonfile is set up correctly for a new TypeScript package:
- The package name follows a scoped naming convention.
- The version is set to
1.0.0for the initial release.- The
mainandtypesfields point to the correct entry point.- The author and license fields are properly set.
- The
devDependenciesanddependenciessections include the necessary packages.apps/agent/pkg/clickhouse/schema/002_create_key_verifications_table.sql (2)
27-28: Skipping the cosmetic change.The addition of blank lines before the closing parenthesis is a cosmetic change that does not affect the functionality of the SQL schema. The necessity of this change depends on the project's coding style guidelines.
15-15: Verify the cardinality assumption for theregioncolumn.The change from
StringtoLowCardinality(String)for theregioncolumn is appropriate if the column is expected to have a small number of distinct values that are frequently repeated.To ensure the cardinality assumption is correct, run the following script:
If the cardinality is high, consider reverting the change to avoid potential performance issues.
apps/agent/pkg/clickhouse/schema/001_create_requests_table.sql (1)
26-28: LGTM!The
service_latencycolumn is well-defined with an appropriate data type and a clear comment explaining the unit of measurement.apps/dashboard/app/(app)/logs/page.tsx (2)
1-5: LGTM!The imports are relevant and the naming is clear.
8-19: LGTM!The function follows good practices for multi-tenancy, data integrity, and error handling.
apps/api/src/pkg/hono/env.ts (1)
32-33: LGTM! Verify the population of the new properties.The addition of
requestStartedAtandworkspaceIdproperties to theHonoEnvtype looks good. The property names are descriptive, and the chosen types are appropriate.Please ensure that:
requestStartedAtis being populated with the correct timestamp when the request starts.workspaceIdis being set with the appropriate identifier when available.apps/api/package.json (1)
39-39: Verify the usage of the new dependency@unkey/clickhouse-zod.The addition of the new dependency
@unkey/clickhouse-zodlooks good. It's managed within the same monorepo workspace, which can help maintain consistency across packages.Please ensure that the new dependency is properly utilized and integrated within the application code. You can use the following script to search for the package import and usage:
Verification successful
Verified: The @unkey/clickhouse-zod package is correctly imported and utilized.
The new dependency
@unkey/clickhouse-zodis properly integrated into the codebase:
- In
apps/dashboard/lib/clickhouse/index.ts, it's used for querying data from Clickhouse.- In
apps/api/src/pkg/analytics.ts, it's used for inserting data into Clickhouse tables.Both implementations include proper error handling with noop options when a Clickhouse URL is not available. The usage aligns with the package's intended purpose of interacting with Clickhouse databases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new dependency `@unkey/clickhouse-zod`. # Test: Search for the package import. Expect: At least one import statement. rg --type typescript -A 5 $'@unkey/clickhouse-zod' # Test: Search for the package usage. Expect: At least one usage of the imported package. rg --type typescript -A 5 $'clickhouse-zod'Length of output: 162
Script:
#!/bin/bash # Description: Verify the usage of the new dependency `@unkey/clickhouse-zod`. # Test: Search for the package import in TypeScript and JavaScript files. echo "Searching for package import:" fd -e ts -e js | xargs rg -n '@unkey/clickhouse-zod' # Test: Search for the package usage in TypeScript and JavaScript files. echo "Searching for package usage:" fd -e ts -e js | xargs rg -n 'clickhouse-zod'Length of output: 588
Script:
#!/bin/bash # Description: Examine the contents of files importing @unkey/clickhouse-zod echo "Contents of apps/dashboard/lib/clickhouse/index.ts:" cat apps/dashboard/lib/clickhouse/index.ts echo -e "\nContents of apps/api/src/pkg/analytics.ts:" cat apps/api/src/pkg/analytics.tsLength of output: 8635
apps/api/src/pkg/env.ts (1)
31-33: LGTM! The optionalCLICKHOUSE_URLvariable is a good addition.The new
CLICKHOUSE_URLvariable is defined as an optional string type, which allows for flexibility in the environment configuration. This change expands the configuration options without introducing any breaking changes.Considerations:
- Ensure that the application logic handles scenarios where the
CLICKHOUSE_URLvariable is not provided. This could involve using default values, falling back to alternative data storage, or implementing graceful error handling.- If the Clickhouse database is a critical component of the application, consider providing clear documentation or error messages to guide users on configuring this variable when necessary.
apps/dashboard/lib/clickhouse/index.ts (1)
6-52: LGTM!The function correctly retrieves API request logs from Clickhouse using the provided library and Zod for validation. The implementation aligns with the PR objective of collecting API metrics.
apps/dashboard/lib/env.ts (1)
40-41: LGTM!The addition of the optional
CLICKHOUSE_URLenvironment variable is a non-breaking change that expands the configuration options for the application. The variable is correctly typed as a string and marked as optional using thez.string().optional()schema. The change follows the existing code style and conventions and does not alter any existing functionality or logic.tools/local/src/main.ts (1)
59-59: LGTM!The addition of the
clickhousecontainer to thestartContainersfunction call for thedashboardcase appears to be a valid change. It aligns with the PR objective of introducing theclickhousecontainer for collecting API metrics.apps/api/src/routes/v1_keys_verifyKey.ratelimit_accuracy.test.ts (1)
34-34: LGTM!The change to extend the
durationvalue for the test case from 5000 to 30000 milliseconds is a valid update. It allows the test to validate the rate limiting behavior over a longer period, which aligns with the comment above the test cases suggesting that the test duration should be at least 10x the duration of the rate limit window.The change does not introduce any correctness issues or bugs.
deployment/docker-compose.yaml (2)
76-76: LGTM!Setting a specific admin user for ClickHouse is a good practice to avoid using the default admin user.
131-131: LGTM!Adding the
clickhouseservice to thedepends_onsection for theapiservice is a good practice to ensure that ClickHouse is ready before theapiservice starts.apps/api/src/pkg/middleware/init.ts (2)
49-49: LGTM!Setting the
requestStartedAtcontext variable to the current timestamp is a good addition for tracking the request processing time. This can be useful for monitoring and optimizing the API performance.
110-114: LGTM!The modification to the
Analyticsinstance initialization allows for flexible configuration of the ClickHouse integration based on the presence of theCLICKHOUSE_URLenvironment variable. This change aligns with the PR objective of introducing a ClickHouse container for collecting API metrics.apps/dashboard/package.json (1)
45-45: Looks good! Verify compatibility and update relevant code.The addition of the
@unkey/clickhouse-zoddependency seems appropriate. Please ensure that:
- The new dependency is compatible with the existing codebase and doesn't introduce any breaking changes.
- Any relevant code (e.g., imports, type definitions, etc.) has been updated to utilize the new dependency correctly.
- The documentation (e.g., README, architecture docs, etc.) has been updated to reflect the new dependency and its usage, if applicable.
apps/api/src/pkg/keys/service.ts (1)
154-154: LGTM!The code segment correctly sets the
workspaceIdin thecobject based on the response received. The nullish coalescing operator is used appropriately to handle the case whereforWorkspaceIdmight be undefined.apps/api/src/pkg/analytics.ts (4)
2-2: Import Clickhouse module for database interactionsImporting
Clickhousefrom"@unkey/clickhouse-zod"is necessary for enabling Clickhouse database operations within theAnalyticsclass.
21-21: Declare private Clickhouse instance variableThe private member
clickhouseis correctly declared to manage interactions with the Clickhouse database.
29-31: Add Clickhouse configuration to constructor optionsIncluding an optional
clickhouseconfiguration in the constructor allows for flexible initialization of the Clickhouse client based on the presence of a Clickhouse URL.
41-47: Initialize Clickhouse instance conditionallyThe Clickhouse instance is initialized based on whether
opts.clickhouse?.urlis provided. If a URL is available, a new Clickhouse client is created with the specified URL; otherwise, a no-operation (noop) client is used. This approach ensures the class behaves correctly regardless of Clickhouse availability.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
internal/clickhouse-zod/src/interface.ts (2)
4-15: LGTM: Well-designed query method with strong typingThe
querymethod is well-structured and provides strong typing using Zod schemas for both input parameters and output results. The use of generics ensures type safety throughout the querying process.The comments provide helpful examples, which is great. Consider adding a brief example for the return type as well, to illustrate how the method is used.
Consider adding an example comment for the return type, like this:
// Returns a function that can be called with params // Example: const result = await query(params)
17-23: LGTM: Well-structured insert method with flexible inputThe
insertmethod is well-designed, using generics with Zod schemas to ensure type safety for the inserted data. It provides flexibility by allowing insertion of either a single event or an array of events.For consistency with the
querymethod and to improve clarity, consider adding comments explaining the usage of this method.Consider adding comments to explain the usage, similar to the
querymethod. For example:insert<TSchema extends z.ZodSchema<any>>(req: { // The name of the table to insert into table: string; // The schema of the data to be inserted // Example: z.object({ id: z.string(), value: z.number() }) schema: TSchema; }): ( // Accept either a single event or an array of events events: z.input<TSchema> | z.input<TSchema>[], ) => Promise<{ executed: boolean; query_id: string }>;internal/clickhouse-zod/src/noop.ts (1)
3-3: LGTM: Class declaration is correct. Consider adding a class-level comment.The
Noopclass correctly implements theClickhouseinterface. This is good for ensuring type safety and adherence to the interface contract.Consider adding a class-level JSDoc comment to explain the purpose of this no-operation implementation. For example:
/** * A no-operation implementation of the Clickhouse interface. * This class can be used for testing or as a placeholder when a real Clickhouse instance is not available. */ export class Noop implements Clickhouse {apps/api/src/pkg/analytics.ts (1)
Line range hint
1-149: Overall review: ClickHouse integration well-implemented with some improvements neededThe integration of ClickHouse into the
Analyticsclass is generally well-implemented. The new methods for inserting data into ClickHouse follow the existing patterns in the file and use zod for schema validation, which is good practice.However, there are a few areas that need attention:
- Error handling: Consider adding error handling for ClickHouse client initialization and data insertion operations.
- Data privacy: Implement data sanitization or redaction for sensitive information before inserting into ClickHouse, especially for the
insertApiRequestmethod.- Code duplication: The
insertKeyVerificationandinsertApiRequestmethods have similar structures. Consider extracting common logic into a reusable function to improve maintainability.To further improve the architecture:
- Consider creating a separate
ClickHouseClientclass to encapsulate all ClickHouse-related operations. This would improve separation of concerns and make theAnalyticsclass more focused.- Implement a logging strategy that allows for easy debugging of ClickHouse operations while respecting data privacy concerns.
- Consider adding a method to check the ClickHouse connection status, which could be useful for health checks or diagnostics.
These improvements will enhance the robustness, maintainability, and security of the ClickHouse integration.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- apps/api/src/pkg/analytics.ts (4 hunks)
- apps/dashboard/lib/clickhouse/index.ts (1 hunks)
- internal/clickhouse-zod/src/client.ts (1 hunks)
- internal/clickhouse-zod/src/index.ts (1 hunks)
- internal/clickhouse-zod/src/interface.ts (1 hunks)
- internal/clickhouse-zod/src/noop.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- apps/dashboard/lib/clickhouse/index.ts
- internal/clickhouse-zod/src/client.ts
- internal/clickhouse-zod/src/index.ts
Additional comments not posted (6)
internal/clickhouse-zod/src/interface.ts (3)
1-1: LGTM: Correct import of Zod typesThe import statement correctly imports the
ztype from the "zod" library using a type-only import. This is a good practice for maintaining clear separation between runtime and type-level code.
3-3: LGTM: Well-defined Clickhouse interfaceThe
Clickhouseinterface is appropriately named and exported, making it available for use throughout the codebase. It provides a clear contract for interacting with ClickHouse, covering essential operations like querying and inserting data.
1-23: Great implementation of ClickHouse interface with strong typingThis file provides a well-structured and type-safe interface for interacting with ClickHouse, which aligns perfectly with the PR objective of "clickhouse container collecting api metrics". The use of Zod schemas for both querying and inserting data ensures runtime validation, which is crucial for maintaining data integrity when collecting metrics.
Key strengths:
- Strong typing using generics and Zod schemas
- Flexible methods for querying and inserting data
- Clear separation of concerns between query definition and execution
The only minor suggestion is to add comments to the
insertmethod for consistency and improved clarity.Overall, this implementation provides a solid foundation for collecting and managing API metrics in ClickHouse.
internal/clickhouse-zod/src/noop.ts (2)
1-2: LGTM: Imports are appropriate and type-safe.The import statements are correctly using the
typekeyword, which is good for type safety in TypeScript. The imports are relevant to the implementation of theNoopclass.
22-38: LGTM: Insert method is well-implemented.The
insertmethod is well-structured and consistent with the no-op nature of the class. Key points:
- Proper handling of both single events and arrays of events.
- Correct use of Zod for schema validation.
- Appropriate error handling for validation failures.
- Consistent return of a mock result.
The implementation provides a good balance of type safety and flexibility.
apps/api/src/pkg/analytics.ts (1)
2-2: LGTM: ClickHouse import added correctlyThe import for ClickHouse is correctly added, which is necessary for the new ClickHouse integration in the
Analyticsclass.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Dependencies
@unkey/clickhouse-zodfor both API and dashboard applications to support ClickHouse interactions.