WIP: Open Source Telemetry #2363
Conversation
|
WalkthroughThe recent updates involve modifications to the Dockerfile and the introduction of a new 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 (
|
|
PR Description updated to latest commit (e6d8242) |
PR Review 🔍
Code feedback:
|
PR Code Suggestions ✨
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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 (3)
- services/workflows-service/package.json (1 hunks)
- services/workflows-service/src/auth/local/local-auth.guard.ts (2 hunks)
- services/workflows-service/src/env.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- services/workflows-service/package.json
Additional comments not posted (3)
services/workflows-service/src/auth/local/local-auth.guard.ts (2)
4-5: Ensure that the imports from@supabase/supabase-jsand '@/env' are used appropriately in the file.
26-26: The return statement correctly passes the result from the parentcanActivatemethod. Good use of explicit type casting to boolean.services/workflows-service/src/env.ts (1)
81-87: The new telemetry-related environment variables are well-defined with appropriate validations and descriptions. Ensure that these variables are documented in the project's environment configuration guide.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- services/workflows-service/src/auth/local/local-auth.guard.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/auth/local/local-auth.guard.ts
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/main.ts (1)
41-41: Use consistent variable naming conventionsThe variable
SupabaseClientis instantiated as an object but is named with an initial uppercase letter, which is typically reserved for classes or constructors in JavaScript/TypeScript. Consider renaming it tosupabaseClientfor consistency.-const SupabaseClient = createClient( +const supabaseClient = createClient(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- services/workflows-service/Dockerfile (1 hunks)
- services/workflows-service/entrypoint.sh (1 hunks)
- services/workflows-service/src/main.ts (2 hunks)
🧰 Additional context used
🪛 Shellcheck
services/workflows-service/entrypoint.sh
[warning] 65-65: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
[warning] 68-68: Quotes/backslashes in this variable will not be respected.
(SC2090)
🔇 Additional comments (3)
services/workflows-service/Dockerfile (2)
35-39: Summary of Dockerfile changesThe modifications to this Dockerfile align well with the PR objectives for integrating telemetry:
- The addition of
entrypoint.shprovides a mechanism for gathering infrastructure details and initializing telemetry.- The switch from
CMDtoENTRYPOINTallows for more flexible container startup and potential runtime argument passing.These changes enhance the application's capabilities while maintaining a clean Docker build process. Ensure that the
entrypoint.shscript is properly implemented to handle both telemetry initialization and application startup.
39-39: Changed container startup methodThe
ENTRYPOINTinstruction has replaced the previousCMDinstruction. This change provides more flexibility in how the container starts and allows for runtime argument passing.Let's verify the contents of the
entrypoint.shscript to ensure it's properly set up:✅ Verification successful
ENTRYPOINT Configuration Verified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Display the contents of entrypoint.sh if [ -f services/workflows-service/entrypoint.sh ]; then cat services/workflows-service/entrypoint.sh else echo "entrypoint.sh does not exist" fiLength of output: 1744
services/workflows-service/src/main.ts (1)
38-38: Avoid logging sensitive dataLogging
infradatamay expose sensitive or confidential information. Ensure that logging this data complies with security policies, or consider removing the log statement.[security]
-console.log(infradata);
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/main.ts (1)
Line range hint
20-48: Summary of telemetry implementation reviewThe introduction of telemetry using Supabase is a valuable addition to the application. However, there are several areas for improvement:
- Infradata initialization: Consider using a more robust and configurable approach for reading the JSON file.
- Asynchronous execution: Move the telemetry code into an async function to avoid top-level await issues.
- Error handling: Implement proper error handling for both file reading and Supabase operations.
- Code organization: Consider moving the telemetry initialization to a separate module or service for better maintainability.
Addressing these points will significantly improve the robustness and maintainability of the telemetry implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/main.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
services/workflows-service/src/main.ts (1)
20-20: LGTM: Supabase client import.The import statement for the Supabase client is correctly placed and uses named import, which is good for tree-shaking.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/main.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
services/workflows-service/src/main.ts (3)
20-20: LGTM: Supabase client import.The import statement for the Supabase client is correctly placed and uses named import, which is good for tree-shaking.
39-39:⚠️ Potential issueImprove infradata initialization for better error handling and conditional loading.
The current approach of using
requirewith an absolute path may lead to portability issues and lacks error handling. Consider the following improvements:
- Use a configurable path or environment variable instead of a hardcoded absolute path.
- Implement error handling for file reading operations.
- Load
infradataonly when telemetry is enabled to optimize performance.Example implementation:
import * as fs from 'fs'; import * as path from 'path'; let infradata; if (env.TELEMETRY_ENABLED) { try { const infraPath = process.env.INFRA_DATA_PATH || '/tmp/infra.json'; const rawData = fs.readFileSync(path.resolve(infraPath), 'utf8'); infradata = JSON.parse(rawData); } catch (error) { console.error('Error reading infra.json:', error); // Handle the error appropriately } }This approach addresses the concerns raised in the previous comments and improves the overall robustness of the code.
50-55:⚠️ Potential issueRefactor telemetry data insertion for proper error handling and asynchronous execution.
The current implementation has several issues:
- It uses an undefined
supabasevariable, which will cause a runtime error.- The insertion is performed outside the telemetry enabled condition.
- It uses
awaitat the top level, which can cause syntax errors in CommonJS modules.- Error logging to console may not be suitable for production.
Consider refactoring the telemetry implementation as follows:
const initializeTelemetry = async () => { if (env.TELEMETRY_ENABLED && env.TELEMETRY_SUPABASE_URL && env.TELEMETRY_SUPABASE_API_KEY) { try { const supabaseClient = createClient( env.TELEMETRY_SUPABASE_URL, env.TELEMETRY_SUPABASE_API_KEY, { db: { schema: 'public' }, }, ); const { error } = await supabaseClient.from('infra').insert([infradata]); if (error) { logger.error('Error inserting infradata into Supabase:', error); } else { logger.info('Telemetry data inserted successfully'); } } catch (error) { logger.error('Unexpected error during Supabase operation:', error); } } }; // Call this function at the beginning of the main function const main = async () => { await initializeTelemetry(); // ... rest of the main function };This refactored version addresses the issues by:
- Moving the telemetry code inside an async function.
- Adding proper error handling for Supabase operations.
- Using the application logger instead of console.
- Calling the telemetry initialization at the beginning of the main function.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- services/workflows-service/Dockerfile (1 hunks)
- services/workflows-service/entrypoint.sh (1 hunks)
- services/workflows-service/src/main.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/main.ts
🧰 Additional context used
🪛 Shellcheck
services/workflows-service/entrypoint.sh
[warning] 65-65: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
[warning] 68-68: Quotes/backslashes in this variable will not be respected.
(SC2090)
🔇 Additional comments (4)
services/workflows-service/Dockerfile (2)
35-35: LGTM: Entrypoint script integration looks goodThe changes to copy the
entrypoint.shscript and set it as the ENTRYPOINT for the production stage are correct and align with the PR objectives for integrating telemetry. This setup allows for gathering infrastructure details and initializing the application with telemetry capabilities.Also applies to: 39-39
Line range hint
21-21: Clarification on CMD instruction in dev stageThe CMD instruction
[ "dumb-init", "npm", "run", "dev", "--host" ]is still present in the development stage. This is correct and doesn't conflict with the new ENTRYPOINT in the production stage. The CMD instruction is used for development purposes, while the ENTRYPOINT is used for the production environment.services/workflows-service/entrypoint.sh (2)
47-49: LGTM: Current time retrieval is correctThe
get_current_timefunction correctly retrieves the current UTC time in ISO 8601 format. This is a standard and widely compatible approach.
59-70:⚠️ Potential issueFix JSON construction and file writing
The current implementation of JSON construction and file writing has several issues:
- The JSON construction using single quotes doesn't properly handle variable expansion.
- The
echocommand for writing to the file doesn't use quotes, which could lead to word splitting and globbing.Apply the following changes to fix these issues:
-infra_json='{"cloudProvider":"'"$cloud_provider"'","tool":"'"$dep_tool"'","hostname":"'"$hostname"'", "currentTime": "'"$currentTime"'"}' -echo "$infra_json" -echo $infra_json > $infra_file +infra_json=$(jq -n \ + --arg cp "$cloud_provider" \ + --arg dt "$dep_tool" \ + --arg hn "$hostname" \ + --arg ct "$currentTime" \ + '{"cloudProvider":$cp,"tool":$dt,"hostname":$hn,"currentTime":$ct}') +echo "$infra_json" +echo "$infra_json" > "$infra_file"This change uses
jqto construct the JSON, which properly handles escaping and quoting. It also ensures that theechocommand uses quotes when writing to the file. Ifjqis not available in your environment, you can use theprintfmethod suggested in the previous review comment.To ensure
jqis available in your environment, you can run:✅ Verification successful
JSON construction and file writing are correctly implemented
The
jqcommand is available in the environment, ensuring proper JSON construction and safe file writing as suggested.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash if ! command -v jq &> /dev/null; then echo "jq is not installed. Please install it or use the printf method for JSON construction." exit 1 fiLength of output: 15
🧰 Tools
🪛 Shellcheck
[warning] 65-65: Quotes/backslashes will be treated literally. Use an array.
(SC2089)
[warning] 68-68: Quotes/backslashes in this variable will not be respected.
(SC2090)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
services/workflows-service/.env.example (1)
37-39: Consider improving telemetry configuration defaults and documentation.
- Consider changing
TELEMETRY_ENABLEDdefault tofalseto make it opt-in rather than opt-out, which is a better practice for privacy-conscious users.- Add comments explaining:
- The purpose of these variables
- Required format for
TELEMETRY_SUPABASE_URL- Instructions on where to obtain
TELEMETRY_SUPABASE_API_KEYApply this diff to improve the configuration:
-TELEMETRY_ENABLED=true -TELEMETRY_SUPABASE_URL="" -TELEMETRY_SUPABASE_API_KEY="" +# Configuration for anonymous usage tracking (optional) +# Set to true to enable telemetry collection +TELEMETRY_ENABLED=false +# Your Supabase project URL (e.g., https://your-project.supabase.co) +TELEMETRY_SUPABASE_URL="" +# Your Supabase API key (get it from your project's API settings) +TELEMETRY_SUPABASE_API_KEY=""services/workflows-service/src/auth/local/local-auth.guard.ts (1)
17-20: Optimization: Remove redundant schema configurationThe schema configuration in the client options is redundant since it's already specified in the query using
from().Apply this diff:
- { - db: { schema: 'public' }, - },deploy/docker-compose-build.yml (1)
62-64: LGTM with security recommendations.The telemetry environment variables are correctly configured. However, consider these security improvements:
- Mark the API key as a secret using Docker Compose secrets management
- Add validation for these variables in the application startup
- Consider adding comments to document the expected format of these variables
Here's how you could implement secrets management:
services: ballerine-workflow-service: environment: TELEMETRY_ENABLED: ${TELEMETRY_ENABLED} TELEMETRY_SUPABASE_URL: ${TELEMETRY_SUPABASE_URL} secrets: - telemetry_api_key secrets: telemetry_api_key: file: ./secrets/telemetry_api_key.txtservices/workflows-service/src/main.ts (1)
38-58: Consider moving telemetry logic to a dedicated service.The telemetry initialization and data insertion logic would be better placed in a dedicated NestJS service. This would improve:
- Separation of concerns
- Testability
- Reusability
- Configuration management
Consider creating a
TelemetryServiceclass:@Injectable() export class TelemetryService { private readonly supabaseClient: SupabaseClient | undefined; constructor( private readonly configService: ConfigService, private readonly logger: AppLoggerService, ) { this.initializeClient(); } private initializeClient() { if ( this.configService.get('TELEMETRY_ENABLED') && this.configService.get('TELEMETRY_SUPABASE_URL') && this.configService.get('TELEMETRY_SUPABASE_API_KEY') ) { this.supabaseClient = createClient( this.configService.get('TELEMETRY_SUPABASE_URL')!, this.configService.get('TELEMETRY_SUPABASE_API_KEY')!, { db: { schema: 'public' }, }, ); } } async trackInfrastructureData() { if (!this.supabaseClient) return; // Implementation here } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- deploy/.env (1 hunks)
- deploy/docker-compose-build.yml (1 hunks)
- services/workflows-service/.env.example (1 hunks)
- services/workflows-service/src/auth/local/local-auth.guard.ts (1 hunks)
- services/workflows-service/src/main.ts (2 hunks)
🔇 Additional comments (4)
deploy/.env (1)
20-22:⚠️ Potential issueConsider disabling telemetry by default and add documentation.
- For privacy considerations, it's recommended to set
TELEMETRY_ENABLED=falseby default, allowing users to explicitly opt-in.- Please add comments explaining:
- The purpose of each telemetry variable
- Required format for
TELEMETRY_SUPABASE_URL- How to obtain the
TELEMETRY_SUPABASE_API_KEYLet's verify the integration of these variables:
services/workflows-service/.env.example (1)
37-39: Verify error handling for empty telemetry credentials.Please ensure proper error handling exists when telemetry is enabled but the URL or API key is empty to prevent runtime errors.
Let's verify the error handling in the code:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about how empty telemetry credentials are handled.
Error handling for empty telemetry credentials is properly implemented
The code has appropriate safeguards in place:
- Environment variables are validated using Zod schema with
.optional()for URL and API key- Telemetry client initialization is wrapped in a condition that checks all three required values:
if (env.TELEMETRY_ENABLED && env.TELEMETRY_SUPABASE_URL && env.TELEMETRY_SUPABASE_API_KEY)- The initialization is also wrapped in try-catch blocks to handle runtime errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how empty telemetry credentials are handled # Test: Search for telemetry initialization code rg -A 10 "TELEMETRY_ENABLED" # Test: Search for error handling patterns around Supabase client initialization ast-grep --pattern 'new createClient($URL, $KEY)'Length of output: 3917
services/workflows-service/src/auth/local/local-auth.guard.ts (1)
4-5: LGTM: Import statements are clean and necessary.services/workflows-service/src/main.ts (1)
20-20: LGTM: Import statement is correctly placed.The Supabase client import is properly added and the package is included in the project dependencies.
pratapalakshmi
force-pushed
the
bal-1975
branch
from
f65a598
to
10961bd
Compare
pratapalakshmi
force-pushed
the
bal-1975
branch
from
5d13262
to
eb4a620
Compare
pratapalakshmi
force-pushed
the
bal-1975
branch
from
7c58cc7
to
d505994
Compare
| if (error) { | ||
| this.logger.error(`Failed to log infra data: ${error.message}`); | ||
| } else { |
There was a problem hiding this comment.
for error handling please use SentryModule and notify error
There was a problem hiding this comment.
do same for other previous function as well
There was a problem hiding this comment.
Can you give a reference in workflow-service i can refer and close it
There was a problem hiding this comment.
sentryService.captureException(error);
pratapalakshmi
force-pushed
the
bal-1975
branch
from
7dfa86d
to
b44db78
Compare
PR Type
enhancement
Description
local-auth.guard.ts.pnpm-lock.yamlandpackage.jsonto include necessary Supabase packages.Changes walkthrough 📝
local-auth.guard.ts
Integrate Supabase for Telemetry Tracking on Loginsservices/workflows-service/src/auth/local/local-auth.guard.ts
env.ts
Add Telemetry Configuration Environment Variablesservices/workflows-service/src/env.ts
pnpm-lock.yaml
Update Dependencies for Supabase Integrationpnpm-lock.yaml
package.json
Add Supabase JS SDK to Package Dependenciesservices/workflows-service/package.json
@supabase/supabase-jsto package dependencies.Summary by CodeRabbit
entrypoint.sh) that gathers and outputs infrastructure details in JSON format to improve visibility into the application's environment.