WIP: Using new log component with Telemere #364
Conversation
WalkthroughThe pull request includes modifications to several files, primarily focusing on enhancing logging functionality and updating dependency versions. The Changes
Assessment against linked issues
Possibly related PRs
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range comments (3)
src/back/api/server.clj (2)
Line range hint
15-22: Consider adding logger dependency to other components.Components like
:http,:router,:database, and:mqmight benefit from having access to the logger component for consistent logging across the system.Consider updating the component dependencies like this:
:logger (component/using (components/new-logger) [:config]) - :http (components/new-http) + :http (component/using (components/new-http) [:logger]) :sentry (component/using (components/new-sentry) [:config]) - :router (components/new-router routes/routes) + :router (component/using (components/new-router routes/routes) [:logger]) - :database (component/using (components/new-database) [:config]) + :database (component/using (components/new-database) [:config :logger]) :mq (component/using (components/new-mq p.workers/workers [{:channel "domains.verification.fired" :event {} ;; every 2 minutes :sleep 120000} {:channel "yml.unified.verification.fired" :event {} ;; every 5 minutes :sleep 300000}] false) - [:config :database :sentry]) + [:config :database :sentry :logger])
Line range hint
44-45: Verify logging during system shutdown.The
stop-system!function still uses the old logging approach withlogs/log. This should be updated to use the new logger component from the system map.Consider updating the shutdown logging:
(defn stop-system! [] - (logs/log :info "stopping system") + (when-let [logger (:logger @system-atom)] + ((:log logger) :info "stopping system")) (swap! system-atom (fn [s] (when s (component/stop s)))))src/back/api/db/mocks.clj (1)
Line range hint
40-136: Consider introducing a logging helper to reduce duplication.The logging implementation is consistent across all operations, which is good. However, the repeated pattern could be simplified using a helper function.
Consider creating a helper function to standardize the logging pattern:
(defn- with-db-trace [op-name ctx f] (logs/trace op-name ctx (let [result (f)] (logs/trace (keyword (str (name op-name) "-result")) (assoc ctx :success (some? result))) result))) ;; Example usage in get-mock-by-id: (defn get-mock-by-id [id db ctx] (with-db-trace ::get-mock-by-id {:mock-id id} #(-> (sql.helpers/select :*) (sql.helpers/from :mock) (sql.helpers/where [:= :id id]) sql/format ((db.utils/build-execute-with-ctx db ctx)) first)))This would:
- Reduce code duplication
- Ensure consistent logging across operations
- Add result logging automatically
- Make it easier to modify logging behavior in one place
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (11)
- deps.edn (1 hunks)
- resources/back/config.edn (1 hunks)
- src/back/api/controllers/mocks.clj (1 hunks)
- src/back/api/controllers/orgs.clj (1 hunks)
- src/back/api/controllers/user.clj (1 hunks)
- src/back/api/db/mocks.clj (3 hunks)
- src/back/api/db/orgs.clj (1 hunks)
- src/back/api/ports/workers.clj (1 hunks)
- src/back/api/server.clj (1 hunks)
- src/dev/api/dev.clj (4 hunks)
- src/dev/utils.clj (1 hunks)
🔇 Additional comments (30)
src/dev/utils.clj (3)
2-6: LGTM! Good improvement in import specificity.Using the specific
com.moclojer.components.logsnamespace instead of the genericcorenamespace improves code clarity and follows better dependency management practices.
Line range hint
18-26: Consider enhancing shutdown logging details.While the basic logging is in place, consider adding more detailed logging about the components being stopped. This would help with debugging and monitoring system shutdown.
(defn stop-system-dev! ([sys-atom] (stop-system-dev! sys-atom true)) ([sys-atom halt-pg?] - (logs/log :info "stopping system") + (logs/log :info "initiating system shutdown") (swap! sys-atom - (fn [s] (when s (component/stop s)))) + (fn [s] + (when s + (logs/log :info "stopping system components") + (let [stopped (component/stop s)] + (logs/log :info "system components stopped successfully") + stopped)))) (when halt-pg? + (logs/log :info "halting PostgreSQL") (pg-emb/halt-pg!))))Additionally, let's verify if this logging implementation is compatible with Telemere:
#!/bin/bash # Description: Check if the logging namespace supports Telemere integration # Expected: Find evidence of Telemere integration in the logging component # Look for Telemere-related code in the logging component rg -A 5 "telemere" src/
Line range hint
8-16: Verify logger initialization in development environment.Since the logger setup was removed from this function, we should verify that the logger is properly initialized through the new
:loggercomponent mentioned in the system map.✅ Verification successful
Let me generate additional shell scripts to gather more information about the logger initialization.
The previous search revealed usage of
com.moclojer.components.logs, so let's examine this namespace and related component initialization. Additionally, we'll look for any logger-related code in the codebase to understand how logging is handled in the development environment.
Based on the shell script results, I can now generate the final response as we have sufficient information about how logging is handled in the codebase.
Logger initialization is properly handled through the component system
The removal of
setup-loggerfromstart-system-dev!is correct as the logger initialization is now handled through the component system. Evidence:
- In
src/dev/api/dev.clj, there's a proper logger component initialization::logger (component/using (components/new-logger) [:config])- The logger component is properly integrated into the component system with a dependency on the
:configcomponent- This approach is consistent across the codebase, as seen in
src/back/api/server.cljwhich uses the same pattern🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if logger initialization is handled by the new logger component # Expected: Find logger component initialization in development-related files # Look for logger component initialization rg -l "(:logger|new-logger)" src/Length of output: 902
Script:
#!/bin/bash # Search for logger-related code and components rg -A 5 "components.*log" src/ # Search for any logger initialization or configuration rg -A 5 "logger|logging" src/ # Look for the specific component namespace fd "components" src/ --exec cat {}Length of output: 14282
src/back/api/ports/workers.clj (3)
17-19: LGTM! Consistent implementation across functionsThe changes maintain consistency with the previous function, following the same pattern for logging and event context handling.
10-12: Verify trace level logging appropriatenessThe change from info to trace level logging might affect operational visibility. Trace level is typically used for very detailed debugging and might be too verbose for production environments.
✅ Verification successful
Let me generate additional scripts to get more context about the logging configuration and usage patterns.
The previous results showed extensive use of trace logging across the codebase, but we need to understand the logging setup and configuration to make a proper assessment of whether the trace level is appropriate.
Based on the gathered information, I can now provide a final response as there's sufficient evidence about the logging patterns in the codebase.
Trace level logging is appropriate for this operation
The codebase shows a consistent pattern where:
- Trace level is extensively used across controllers, DB operations, and worker components for detailed operation tracking
- The application has proper logging configuration with
ch.qos.logback/logback-classic- The default logging level is set to
:infoin the configuration ((components/setup-logger [["*" :info]])), which means trace logs can be enabled when needed without affecting normal production visibility- The change aligns with the codebase's logging strategy where trace is used for detailed operation tracking while info is reserved for significant state changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check logging levels across the codebase # Expected: Consistent use of trace level for similar operations # Search for all logging calls to understand the logging strategy rg "logs/(trace|debug|info|warn|error)" --type clojure # Search for any logging configuration that might filter trace logs rg -i "log.*level|level.*log" --type clojureLength of output: 1931
Script:
#!/bin/bash # Let's check for logging configuration files and imports rg -l "logs|logging" --type clojure # Look for any logging setup or configuration code ast-grep --pattern '(ns $_ (:require [$$$ [logs :as logs] $$$]))' # Check for any logging configuration in project configuration files fd -e edn -e clj -e properties -x cat {} \; | rg -i "log"Length of output: 10932
2-5: Verify Telemere integration with logging componentThe addition of Telemere import aligns with the PR objectives. However, let's verify that the logging component is properly configured to use Telemere.
src/back/api/controllers/user.clj (1)
2-7: LGTM: Clean namespace organizationThe namespace declaration is well-organized and properly includes the new logging component.
src/dev/api/dev.clj (3)
2-8: LGTM: Required imports added correctlyThe new imports for components are properly organized and necessary for the logger integration.
55-57: LGTM: Improved comment readabilityThe added comment marker improves the visual separation of code sections.
18-18: LGTM: Logger component properly integratedThe logger component is correctly configured with its dependency on
:configand follows the component system patterns.✅ Verification successful
Let me gather more information about the logger component implementation to ensure it's consistently configured.
Let me check one more thing to verify the logger component's implementation and usage pattern.
Based on the gathered information, I can now provide the final response:
Logger component integration is correctly configured
The logger component is consistently configured across the codebase:
- It's properly initialized with
:configdependency in both development (src/dev/api/dev.clj) and production (src/back/api/server.clj) environments- The logger is widely and consistently used throughout the codebase via
com.moclojer.components.logsnamespace- All services (yaml-generator, job-ops, cloud-ops, back-api) follow the same pattern for logger integration
- The component follows the standard component lifecycle pattern with proper dependency injection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify logger component configuration across the codebase # Description: Check if the logger component is consistently configured # Check for logger component configuration patterns rg -A 5 "new-logger|:logger"Length of output: 1207
Script:
#!/bin/bash # Check the logger component implementation ast-grep --pattern 'defrecord Logger [$$$] $$$' # Check for logger component usage patterns rg -B 2 -A 5 "require.*components|:require.*components"Length of output: 10840
Script:
#!/bin/bash # Check for logger component implementation and usage rg -B 2 -A 5 "com\.moclojer\.components\.logs" # Check the actual logger component source if available fd -g "logs.clj" -x cat {}Length of output: 11953
src/back/api/server.clj (2)
15-15: LGTM: Logger component integration looks correct.The new logger component is properly integrated into the system map with the correct dependency on
:config.
Line range hint
1-67: Verify error handling during system startup.The system startup process should handle scenarios where the logger component fails to initialize.
Let's check if there are any error handling patterns in other components:
resources/back/config.edn (2)
28-32: Verify OpenSearch configuration integration with Telemere.Since this PR is about integrating with Telemere for logging, please ensure that these OpenSearch settings align with Telemere's requirements and are properly utilized by the logging component.
28-29:⚠️ Potential issueStrengthen security by requiring production credentials.
The default username and password "foobar" could pose a security risk if accidentally used in production. Consider making these credentials mandatory in production environments.
Modify the configuration to enforce credentials in production:
-:opensearch {:username #or [#env OPENSEARCH_USERNAME "foobar"] - :password #or [#env OPENSEARCH_PASSWORD "foobar"] +:opensearch {:username #profile {:dev #or [#env OPENSEARCH_USERNAME "foobar"] + :prod #env OPENSEARCH_USERNAME} + :password #profile {:dev #or [#env OPENSEARCH_PASSWORD "foobar"] + :prod #env OPENSEARCH_PASSWORD}Let's verify if there are any hardcoded credentials in the codebase:
src/back/api/controllers/orgs.clj (3)
11-14: Well-structured logging implementation!The logging changes follow a consistent pattern across all functions:
- Using namespace-qualified keywords for operation identification
- Including relevant context data in structured format
- Proper placement before the actual operation
This approach will significantly improve observability and debugging capabilities.
Also applies to: 18-21, 28-31, 35-38, 44-47, 53-55, 67-70, 78-81
1-84: Verify complete implementation of Telemere logging.While the logging changes in this file look good, let's verify that all necessary components are in place for the Telemere integration:
- Proper configuration in the logging component
- Consistent usage across other controllers
#!/bin/bash # Description: Verify complete Telemere implementation # Test 1: Check if all controllers use the new logging pattern echo "Checking controllers for consistent logging pattern:" rg "logs/(trace|debug|info|warn|error)" --type clojure src/back/api/controllers/ # Test 2: Check logging component configuration echo -e "\nChecking logging component configuration:" fd -t f -e clj -e edn . | xargs rg -l "telemere"
2-7: Verify Telemere usage in the codebase.While Telemere is imported, it doesn't appear to be directly used in this file. Let's verify if it's being used through the logging component.
✅ Verification successful
Let me gather more information about the logging component and Telemere usage to make a conclusive verification.
Based on the gathered information, I can now provide a conclusive response about the Telemere usage.
Telemere import is correctly used through the logging component
The verification shows that:
- All controller files consistently use
logs/tracefor logging- Both
orgs.cljandworkers.cljimport Telemere with the same aliast- The logging pattern is uniform across the codebase, indicating a standardized approach
The import of
taoensso.telemereis justified as it's part of the logging infrastructure update mentioned in the PR objectives. While Telemere isn't directly called with thet/prefix, it's being used through the standardized logging component (logs/trace).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how Telemere is being used across the codebase # Test 1: Check direct usage of Telemere echo "Checking direct Telemere usage:" rg "t/\w+" --type clojure # Test 2: Check if Telemere is used in the logging component echo -e "\nChecking Telemere usage in logging component:" rg -l "taoensso.telemere" --type clojureLength of output: 1536
Script:
#!/bin/bash # Check the logging component implementation and Telemere integration echo "Checking logging component implementation:" rg -A 5 "taoensso.telemere" --type clojure # Check if Telemere is used through the logging namespace echo -e "\nChecking logs namespace usage:" fd -t f logs.clj -x cat {} # Check workers implementation since it also imports Telemere echo -e "\nChecking workers implementation:" cat src/back/api/ports/workers.cljLength of output: 2069
src/back/api/db/orgs.clj (3)
1-7: LGTM! Namespace declaration and imports are well-organized.The addition of the logging component import is properly placed and follows consistent aliasing conventions.
26-34: LGTM! Consistent logging implementation across all database operations.The trace logging implementation follows a consistent pattern throughout all functions:
- Appropriate namespace-qualified keywords for operation identification
- Relevant context data including IDs
- Logging placed before the database operations
This consistency will make log analysis and debugging easier.
Also applies to: 40-48, 54-62, 68-77, 83-89, 95-104, 110-119
6-6: Verify Telemere integration in the logging component.Since this PR aims to update logging to use Telemere, let's verify that
com.moclojer.components.logsis properly integrated with Telemere.src/back/api/db/mocks.clj (2)
1-7: LGTM: Namespace updated with new logging component.The addition of the Telemere logging component import is consistent with the PR objectives.
Line range hint
1-136: Verify complete migration to new logging component.The implementation successfully integrates the new Telemere logging component. Let's verify no old logging remains.
✅ Verification successful
Let me analyze the results and generate a final response.
Old logging patterns found, but not in core backend code
The scan reveals that old logging patterns (
println,prn) are only present in:
- Development and build files (
build.clj,src/dev/core.cljs)- Frontend code (
src/front/app/*)The file under review (
src/back/api/db/mocks.clj) and related backend files have successfully migrated to the new Telemere logging component. The remaining instances of old logging patterns are in non-critical paths and typically used for development purposes, which is acceptable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining old logging patterns in the codebase # that haven't been migrated to the new Telemere component # Look for common logging patterns that should be replaced echo "Checking for old logging patterns in Clojure files..." rg -t clojure "(println|prn|log/debug|log/info|log/warn|log/error)" \ --glob '!test/**' \ --glob '!dev/**'Length of output: 1427
deps.edn (3)
24-28: LGTM! Comprehensive logging setup.The logging dependencies are well-structured with:
- Logback for underlying logging infrastructure
- Timbre for Clojure-friendly logging
- JSON appender for structured logging
- SLF4J bridge for compatibility
2-77: LGTM! Clean and consistent formatting.The deps.edn file is well-organized with:
- Logical grouping of dependencies
- Consistent indentation
- Clear separation of frontend and backend dependencies
- Well-structured aliases
12-12: Verify the components upgrade impact.The version update of
com.moclojer/componentsfrom0.1.0to0.1.4appears to be related to the new Telemere logging functionality. However, we should verify that this version is compatible with the current codebase.✅ Verification successful
Let me gather more specific information about the logging usage to ensure there are no breaking changes.
The previous results showed that the v0.1.4 tag exists and identified files using the logging namespace. Now, I want to examine the actual logging calls and configurations to ensure compatibility with the new version. This will help us determine if there are any breaking changes in the logging patterns that need to be addressed.
Let me try one more verification to check the actual logging function usage in those specific files we found:
This will help us understand how the logging functions are actually being used in the files that import the logging namespace, and if there are any documented changes that might affect the upgrade.
Based on the gathered information, I can now provide a final response:
The components upgrade appears safe to proceed
The verification shows that:
- The target version 0.1.4 exists in the moclojer/components repository
- Current logging usage is simple and follows a consistent pattern using only basic logging functions:
- All calls use the standard
logs/logfunction with a log level and context- No usage of advanced features that might be affected by breaking changes
The logging implementation appears straightforward and stable across the codebase, primarily used for informational logging with consistent parameters (level and context). The upgrade from 0.1.0 to 0.1.4 should be safe as it maintains compatibility with this basic logging pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of components upgrade # 1. Check if the new version exists in the repository # 2. Look for any breaking changes in the logging namespace usage # Check if the version tag exists in the components repository gh repo view moclojer/components >/dev/null 2>&1 && \ echo "Checking version tag v0.1.4 in moclojer/components..." && \ gh api -H "Accept: application/vnd.github+json" \ repos/moclojer/components/git/refs/tags/v0.1.4 # Search for logging namespace usage patterns that might need updates echo "Checking logging namespace usage patterns..." rg -l 'com\.moclojer\.components\.logs' # Look for potential breaking changes in logging calls echo "Checking logging function calls..." ast-grep --pattern 'log/$_($$$)'Length of output: 1554
Script:
#!/bin/bash # Check specific logging function calls and their patterns echo "Checking logging patterns in Clojure files..." rg -A 2 'components\.logs/\w+' # Check for any require/import statements of the logging namespace echo -e "\nChecking logging namespace imports..." rg '(:require.*com\.moclojer\.components\.logs)' # Look for any configuration related to logging echo -e "\nChecking logging configuration patterns..." rg -A 3 '(:logs|logging-config)'Length of output: 586
Script:
#!/bin/bash # Check the content of files that import the logging namespace echo "Checking yaml_generator/ports/workers.clj logging usage:" cat src/yaml_generator/ports/workers.clj echo -e "\nChecking job_ops/controllers/mocks.clj logging usage:" cat src/job_ops/controllers/mocks.clj # Also check the changelog or release notes for breaking changes echo -e "\nChecking for CHANGELOG.md in the current repository:" fd -g "CHANGELOG.md" --exec cat {}Length of output: 4115
src/back/api/controllers/mocks.clj (5)
2-7: LGTM: Import changes align with PR objectives.The addition of
com.moclojer.components.logsimport is consistent with the PR's goal of updating logging usage.
37-40: LGTM: Appropriate logging context.The trace logging with mock-wildcard provides sufficient context for debugging availability checks.
70-72: LGTM: Appropriate trace logging for read operation.The trace logging with user-id provides adequate context for this read operation.
125-128: LGTM: Consistent logging pattern.The trace logging follows the established pattern for read operations.
47-51: Verify content-hash uniqueness across updates.Good use of content-hash to avoid logging sensitive data. Let's verify that the hash implementation provides sufficient uniqueness for tracking changes.
✅ Verification successful
Let me gather more information about the hash implementation to ensure it's suitable for tracking changes.
Let me try one more search to specifically look for the hash function usage in Clojure files:
Based on the search results, I can now provide the final response:
The hash implementation is safe and appropriate for change tracking
The code uses Clojure's built-in
hashfunction, which is a stable implementation that provides consistent hash values for equal contents. Since there are no custom hash implementations found in the codebase, and the function is used directly fromclojure.core, it guarantees:
- Consistent hash values across different runs
- Good distribution of hash values for different contents
- Thread-safe operation
The implementation is suitable for tracking content changes in mocks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of hash function to ensure consistent implementation rg "content-hash|hash\s+content" -A 2 -B 2Length of output: 426
Script:
#!/bin/bash # Search for hash function implementation or imports rg "import.*hash|def.*hash" -A 2 # Look for hash function usage in test files rg "hash.*content" test/ -A 2 -B 2 # Search for clojure.core/hash references ast-grep --pattern '(hash $_)'Length of output: 467
Script:
#!/bin/bash # Search for hash function in Clojure files fd '\.clj$' -x rg -l 'hash' {} # Look for specific hash function implementation or configuration rg "hash.*function|hash.*config" -A 2 -B 2 # Check if there are any custom hash implementations ast-grep --pattern '(defn hash'Length of output: 175
| (logs/trace | ||
| ::username-available? | ||
| {:username username} | ||
| (nil? (db.customers/get-by-username username database ctx)))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding error case logging
The trace logging is well-placed and includes relevant context. However, consider logging the result when username is not available for better debugging.
(logs/trace
::username-available?
{:username username}
- (nil? (db.customers/get-by-username username database ctx))))
+ (let [result (db.customers/get-by-username username database ctx)]
+ (when result
+ (logs/trace ::username-taken {:username username}))
+ (nil? result)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (logs/trace | |
| ::username-available? | |
| {:username username} | |
| (nil? (db.customers/get-by-username username database ctx)))) | |
| (logs/trace | |
| ::username-available? | |
| {:username username} | |
| (let [result (db.customers/get-by-username username database ctx)] | |
| (when result | |
| (logs/trace ::username-taken {:username username})) | |
| (nil? result))) |
| (logs/trace | ||
| ::get-users-by-org-id | ||
| {:org-id org-id} | ||
| (map | ||
| adapter.customers/->wire | ||
| (db.customers/get-by-org-id org-id database ctx)))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add result logging
Consider logging the number of users found for better observability.
(logs/trace
::get-users-by-org-id
{:org-id org-id}
- (map
- adapter.customers/->wire
- (db.customers/get-by-org-id org-id database ctx))))
+ (let [users (db.customers/get-by-org-id org-id database ctx)
+ wire-users (map adapter.customers/->wire users)]
+ (logs/trace ::users-found {:org-id org-id
+ :count (count wire-users)})
+ wire-users))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (logs/trace | |
| ::get-users-by-org-id | |
| {:org-id org-id} | |
| (map | |
| adapter.customers/->wire | |
| (db.customers/get-by-org-id org-id database ctx)))) | |
| (logs/trace | |
| ::get-users-by-org-id | |
| {:org-id org-id} | |
| (let [users (db.customers/get-by-org-id org-id database ctx) | |
| wire-users (map adapter.customers/->wire users)] | |
| (logs/trace ::users-found {:org-id org-id | |
| :count (count wire-users)}) | |
| wire-users)) |
| (logs/trace | ||
| ::edit-user! | ||
| {:user-id user-id | ||
| :new-username username} | ||
| (-> {:customer/uuid user-id} | ||
| (logic.customers/add-username username) | ||
| (db.customers/update! database ctx) | ||
| (adapter.customers/->wire))) | ||
| (adapter.customers/->wire)))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error logging
Consider adding validation for the username and logging for update success/failure.
(logs/trace
::edit-user!
{:user-id user-id
:new-username username}
- (-> {:customer/uuid user-id}
- (logic.customers/add-username username)
- (db.customers/update! database ctx)
- (adapter.customers/->wire))))
+ (try
+ (when-not (logic.customers/valid-username? username)
+ (let [error {:cause :invalid-username
+ :user-id user-id
+ :username username}]
+ (logs/trace ::invalid-username error)
+ (throw (ex-info "Invalid username format" error))))
+ (let [result (-> {:customer/uuid user-id}
+ (logic.customers/add-username username)
+ (db.customers/update! database ctx)
+ (adapter.customers/->wire))]
+ (logs/trace ::user-updated {:user-id user-id :result result})
+ result)
+ (catch Exception e
+ (logs/trace ::user-update-failed {:user-id user-id
+ :error (.getMessage e)})
+ (throw e))))Committable suggestion was skipped due to low confidence.
| (logs/trace | ||
| ::get-user-by-external-id | ||
| {:user-id id} | ||
| (db.customers/get-by-external-id id database ctx))) |
There was a problem hiding this comment.
Add consistent error handling
This function should handle the case where a user is not found, similar to get-user-by-id.
(logs/trace
::get-user-by-external-id
{:user-id id}
- (db.customers/get-by-external-id id database ctx)))
+ (if-let [user (db.customers/get-by-external-id id database ctx)]
+ user
+ (let [error {:status-code 412
+ :cause :invalid-external-id
+ :external-id id
+ :context ctx}]
+ (logs/trace ::user-not-found-by-external-id error)
+ (throw (ex-info "No user with given external id was found" error)))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (logs/trace | |
| ::get-user-by-external-id | |
| {:user-id id} | |
| (db.customers/get-by-external-id id database ctx))) | |
| (logs/trace | |
| ::get-user-by-external-id | |
| {:user-id id} | |
| (if-let [user (db.customers/get-by-external-id id database ctx)] | |
| user | |
| (let [error {:status-code 412 | |
| :cause :invalid-external-id | |
| :external-id id | |
| :context ctx}] | |
| (logs/trace ::user-not-found-by-external-id error) | |
| (throw (ex-info "No user with given external id was found" error))))) |
| (logs/trace | ||
| ::get-user-by-id | ||
| {:user-id id} | ||
| (adapter.customers/->wire | ||
| (or (db.customers/get-by-id id database ctx) | ||
| (throw (ex-info "No user with given id was found" | ||
| {:status-code 412 | ||
| :cause :invalid-id | ||
| :value (assoc ctx :user-id id)}))))) | ||
| :value (assoc ctx :user-id id)})))))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging
While the trace logging is good, consider these improvements:
- Add error logging when user is not found
- Restructure error map for better clarity
(logs/trace
::get-user-by-id
{:user-id id}
(adapter.customers/->wire
(or (db.customers/get-by-id id database ctx)
- (throw (ex-info "No user with given id was found"
- {:status-code 412
- :cause :invalid-id
- :value (assoc ctx :user-id id)})))))
+ (let [error {:status-code 412
+ :cause :invalid-id
+ :user-id id
+ :context ctx}]
+ (logs/trace ::user-not-found error)
+ (throw (ex-info "No user with given id was found" error))))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (logs/trace | |
| ::get-user-by-id | |
| {:user-id id} | |
| (adapter.customers/->wire | |
| (or (db.customers/get-by-id id database ctx) | |
| (throw (ex-info "No user with given id was found" | |
| {:status-code 412 | |
| :cause :invalid-id | |
| :value (assoc ctx :user-id id)}))))) | |
| :value (assoc ctx :user-id id)})))))) | |
| (logs/trace | |
| ::get-user-by-id | |
| {:user-id id} | |
| (adapter.customers/->wire | |
| (or (db.customers/get-by-id id database ctx) | |
| (let [error {:status-code 412 | |
| :cause :invalid-id | |
| :user-id id | |
| :context ctx}] | |
| (logs/trace ::user-not-found error) | |
| (throw (ex-info "No user with given id was found" error)))))) |
| (logs/trace | ||
| ::publish-mock! | ||
| {:mock-id id} | ||
| (-> (db.mocks/get-mock-by-id id database ctx) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider standardizing success/failure logging across operations.
Both publish and unpublish operations would benefit from consistent success/failure logging patterns.
;; Add to both publish-mock! and unpublish-mock!:
+ (logs/trace
+ ::{un}publish-mock-success!
+ {:mock-id id
+ :status "success"})Also applies to: 92-95
| (logs/trace | ||
| ::update-mock-dns-status! | ||
| {:mock-domain domain | ||
| :new-status new-status} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding error tracking for status updates.
Both DNS and unification status updates would benefit from error tracking in logs.
;; Add to both status update functions:
+ (try
+ ;; existing update logic
+ (catch Exception e
+ (logs/error
+ ::{dns|unif}-status-update-failed!
+ {:mock-id id
+ :new-status new-status
+ :error (.getMessage e)}
+ e)))Also applies to: 155-158
src/back/api/controllers/mocks.clj
Outdated
| (logs/trace | ||
| ::publish-mock! | ||
| {:mock-id id} | ||
| (-> (db.mocks/get-mock-by-id id database ctx) | ||
| logic.mocks/publish | ||
| (db.mocks/update! database ctx) | ||
| (adapter.mocks/->wire) | ||
| :id | ||
| (ports.producers/generate-single-yml! mq ctx))) | ||
| true) | ||
|
|
||
| (defn unpublish-mock! | ||
| [id {:keys [database mq]} ctx] | ||
| (-> (db.mocks/get-mock-by-id id database ctx) | ||
| (logs/trace | ||
| ::unpublish-mock! | ||
| {:mock-id id} | ||
| (-> (db.mocks/get-mock-by-id id database ctx) | ||
| logic.mocks/unpublish | ||
| (db.mocks/update! database ctx) | ||
| (adapter.mocks/->wire) | ||
| :id | ||
| (ports.producers/generate-single-yml! mq ctx)) | ||
| (ports.producers/generate-single-yml! mq ctx))) | ||
| true) | ||
|
|
||
| (defn delete-mock! | ||
| [{:keys [user-id]} id {:keys [database mq]} ctx] | ||
| (logs/log :info "deleting mock" | ||
| :ctx (assoc ctx :mock-id id)) | ||
| (if-let [{:keys [id org-id user-id] :as ?mock} | ||
| (some-> (db.mocks/get-mocks user-id database ctx) | ||
| (logic.mocks/filter-by-id id) | ||
| (adapter.mocks/->wire))] | ||
| (do | ||
| (db.mocks/delete-mock-by-id id database ctx) | ||
| (ports.producers/delete-single-yml! id (or org-id user-id) mq ctx) | ||
| (ports.producers/delete-domain! (logic.mocks/pack-domain ?mock) mq ctx) | ||
| true) | ||
| (logs/trace | ||
| ::delete-mock! | ||
| {:user-id user-id | ||
| :mock-id id} | ||
| (do | ||
| (db.mocks/delete-mock-by-id id database ctx) | ||
| (ports.producers/delete-single-yml! id (or org-id user-id) mq ctx) | ||
| (ports.producers/delete-domain! (logic.mocks/pack-domain ?mock) mq ctx) | ||
| true)) | ||
| (throw (ex-info "No mock found with given id" | ||
| {:status-code 400 | ||
| :cause :invalid-id | ||
| :value :id})))) | ||
|
|
||
| (defn get-mock-publication-status | ||
| [id db ctx] | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str id)) db ctx)] | ||
| (logs/trace | ||
| ::get-mock-pub-status | ||
| {:mock-id id} | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str id)) db ctx)] | ||
| (-> (adapter.mocks/->wire mock) | ||
| (select-keys [:dns-status :unification-status])) | ||
| (throw (ex-info "No mock found with given id" | ||
| {:status-code 400 | ||
| :cause :invalid-id | ||
| :value id})))) | ||
| :value id}))))) | ||
|
|
||
| (defn update-mock-dns-status! | ||
| [domain new-status db ctx] | ||
| (if-let [mock (-> (logic.mocks/unpack-domain domain) | ||
| (logic.mocks/->db-by-wildcard) | ||
| (db.mocks/get-mock-by-wildcard db ctx))] | ||
| (-> (logic.mocks/update-dns-status mock new-status) | ||
| (select-keys [:mock/id :mock/dns_status]) | ||
| (db.mocks/update! db ctx)) | ||
| (logs/trace | ||
| ::update-mock-dns-status! | ||
| {:mock-domain domain | ||
| :new-status new-status} | ||
| (-> (logic.mocks/update-dns-status mock new-status) | ||
| (select-keys [:mock/id :mock/dns_status]) | ||
| (db.mocks/update! db ctx))) | ||
| (throw (ex-info "No mock found with given domain" | ||
| {:cause :invalid-domain | ||
| :value domain})))) | ||
|
|
||
| (defn update-mock-unification-status! | ||
| [mock-id new-status db ctx] | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str mock-id)) db ctx)] | ||
| (-> (logic.mocks/update-unification-status mock new-status) | ||
| (select-keys [:mock/id :mock/unification_status]) | ||
| (db.mocks/update! db ctx)) | ||
| (logs/trace | ||
| ::update-mock-unif-status! | ||
| {:mock-id mock-id | ||
| :new-status new-status} | ||
| (-> (logic.mocks/update-unification-status mock new-status) | ||
| (select-keys [:mock/id :mock/unification_status]) | ||
| (db.mocks/update! db ctx))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Overall logging implementation review.
The logging changes show good consistency and align well with the PR objectives. Some suggestions for enhancement:
- Consider implementing a consistent pattern for success/failure logging across all operations
- Consider elevating certain operations (like deletions) to INFO level for audit purposes
- Consider adding error tracking for critical operations
These changes provide good observability while maintaining clean code structure.
| (logs/trace | ||
| ::delete-mock! | ||
| {:user-id user-id | ||
| :mock-id id} | ||
| (do |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding audit logging for deletions.
While the trace logging is good, deletions should potentially be logged at INFO level for audit purposes.
- (logs/trace
+ (logs/info
::delete-mock!
{:user-id user-id
:mock-id id})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (logs/trace | |
| ::delete-mock! | |
| {:user-id user-id | |
| :mock-id id} | |
| (do | |
| (logs/info | |
| ::delete-mock! | |
| {:user-id user-id | |
| :mock-id id} | |
| (do |
| (logs/trace | ||
| ::create-mock! | ||
| {:mock-id (:id mock) | ||
| :user-id user-id} | ||
| (let [owner (assoc {:user-id (parse-uuid (str user-id))} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding operation outcome to logs.
The structured logging with mock-id and user-id provides good context. Consider adding a second trace log after the operation completes to track success/failure.
(logs/trace
::create-mock!
{:mock-id (:id mock)
:user-id user-id}
+ ;; Add at the end of the function:
+ (logs/trace
+ ::create-mock-success!
+ {:mock-id (:id mock)
+ :user-id user-id})Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/back/api/controllers/mocks.clj (1 hunks)
🔇 Additional comments (2)
src/back/api/controllers/mocks.clj (2)
2-7: LGTM: Import changes align with PR objectives.The addition of
com.moclojer.components.logsimport is appropriate for integrating the new logging component.
167-182: 🧹 Nitpick (assertive)Review meta-programming approach for function wrapping.
While the automatic function wrapping is clever, there are several concerns to address:
- Runtime function modification can complicate debugging and hot reloading
- The context map creation might fail with destructured arguments
- No error handling in the wrapped logging
- Potential performance impact from double function evaluation
Let's check for destructured arguments that might break the context map creation:
Consider these alternatives:
- Use a macro to wrap functions at compile time
- Create a higher-order function for wrapping
- Use aspect-oriented programming libraries like robert/hooke
Example using a higher-order function:
(defn wrap-with-logging [f fname] (fn [& args] (try (let [context (-> (zipmap (map keyword (take (- (count args) 2) args)) args) (dissoc :components :ctx))] (logs/trace fname context) (apply f args)) (catch Exception e (binding [*out* *err*] (println "Logging error in" fname ":" (.getMessage e))) (apply f args))))) ;; Usage: (def create-mock! (wrap-with-logging create-mock! ::create-mock!))
src/back/api/controllers/mocks.clj
Outdated
| (logs/trace | ||
| ::publish-mock! | ||
| {:mock-id id} | ||
| (-> (db.mocks/get-mock-by-id id database ctx) | ||
| logic.mocks/publish | ||
| (db.mocks/update! database ctx) | ||
| (adapter.mocks/->wire) | ||
| :id | ||
| (ports.producers/generate-single-yml! mq ctx))) | ||
| true) | ||
|
|
||
| (defn unpublish-mock! | ||
| [id {:keys [database mq]} ctx] | ||
| (-> (db.mocks/get-mock-by-id id database ctx) | ||
| (logs/trace | ||
| ::unpublish-mock! | ||
| {:mock-id id} | ||
| (-> (db.mocks/get-mock-by-id id database ctx) | ||
| logic.mocks/unpublish | ||
| (db.mocks/update! database ctx) | ||
| (adapter.mocks/->wire) | ||
| :id | ||
| (ports.producers/generate-single-yml! mq ctx)) | ||
| (ports.producers/generate-single-yml! mq ctx))) | ||
| true) | ||
|
|
||
| (defn delete-mock! | ||
| [{:keys [user-id]} id {:keys [database mq]} ctx] | ||
| (logs/log :info "deleting mock" | ||
| :ctx (assoc ctx :mock-id id)) | ||
| (if-let [{:keys [id org-id user-id] :as ?mock} | ||
| (some-> (db.mocks/get-mocks user-id database ctx) | ||
| (logic.mocks/filter-by-id id) | ||
| (adapter.mocks/->wire))] | ||
| (do | ||
| (db.mocks/delete-mock-by-id id database ctx) | ||
| (ports.producers/delete-single-yml! id (or org-id user-id) mq ctx) | ||
| (ports.producers/delete-domain! (logic.mocks/pack-domain ?mock) mq ctx) | ||
| true) | ||
| (logs/trace | ||
| ::delete-mock! | ||
| {:user-id user-id | ||
| :mock-id id} | ||
| (do | ||
| (db.mocks/delete-mock-by-id id database ctx) | ||
| (ports.producers/delete-single-yml! id (or org-id user-id) mq ctx) | ||
| (ports.producers/delete-domain! (logic.mocks/pack-domain ?mock) mq ctx) | ||
| true)) | ||
| (throw (ex-info "No mock found with given id" | ||
| {:status-code 400 | ||
| :cause :invalid-id | ||
| :value :id})))) | ||
|
|
||
| (defn get-mock-publication-status | ||
| [id db ctx] | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str id)) db ctx)] | ||
| (logs/trace | ||
| ::get-mock-pub-status | ||
| {:mock-id id} | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str id)) db ctx)] | ||
| (-> (adapter.mocks/->wire mock) | ||
| (select-keys [:dns-status :unification-status])) | ||
| (throw (ex-info "No mock found with given id" | ||
| {:status-code 400 | ||
| :cause :invalid-id | ||
| :value id})))) | ||
| :value id}))))) | ||
|
|
||
| (defn update-mock-dns-status! | ||
| [domain new-status db ctx] | ||
| (if-let [mock (-> (logic.mocks/unpack-domain domain) | ||
| (logic.mocks/->db-by-wildcard) | ||
| (db.mocks/get-mock-by-wildcard db ctx))] | ||
| (-> (logic.mocks/update-dns-status mock new-status) | ||
| (select-keys [:mock/id :mock/dns_status]) | ||
| (db.mocks/update! db ctx)) | ||
| (logs/trace | ||
| ::update-mock-dns-status! | ||
| {:mock-domain domain | ||
| :new-status new-status} | ||
| (-> (logic.mocks/update-dns-status mock new-status) | ||
| (select-keys [:mock/id :mock/dns_status]) | ||
| (db.mocks/update! db ctx))) | ||
| (throw (ex-info "No mock found with given domain" | ||
| {:cause :invalid-domain | ||
| :value domain})))) | ||
|
|
||
| (defn update-mock-unification-status! | ||
| [mock-id new-status db ctx] | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str mock-id)) db ctx)] | ||
| (-> (logic.mocks/update-unification-status mock new-status) | ||
| (select-keys [:mock/id :mock/unification_status]) | ||
| (db.mocks/update! db ctx)) | ||
| (logs/trace | ||
| ::update-mock-unif-status! | ||
| {:mock-id mock-id | ||
| :new-status new-status} | ||
| (-> (logic.mocks/update-unification-status mock new-status) | ||
| (select-keys [:mock/id :mock/unification_status]) | ||
| (db.mocks/update! db ctx))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider wrapping trace logging in try-catch blocks.
While the trace logging implementation is consistent, it's important to ensure that logging failures don't affect the main functionality. Consider wrapping the trace calls in try-catch blocks, especially for critical operations.
Example implementation:
(defn create-mock!
[user-id mock {:keys [database mq]} ctx]
- (logs/trace
- ::create-mock!
- {:mock-id (:id mock)
- :user-id user-id}
+ (try
+ (logs/trace
+ ::create-mock!
+ {:mock-id (:id mock)
+ :user-id user-id})
+ (catch Exception e
+ ; Swallow logging errors but maybe log to stderr
+ (binding [*out* *err*]
+ (println "Logging error in create-mock!:" (.getMessage e)))))Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/back/api/controllers/mocks.clj(1 hunks)
🔇 Additional comments (1)
src/back/api/controllers/mocks.clj (1)
1-8: LGTM: Clean namespace organization with new logging component.
The addition of com.moclojer.components.logs aligns with the PR objectives of updating logging usage.
src/back/api/controllers/mocks.clj
Outdated
| ;; Iterate over *ns* functions and replace them with themselves within | ||
| ;; a `logs/trace` call that uses each function's arglist as context. | ||
| (doseq [[fsym func] (ns-publics *ns*)] | ||
| (let [args (first (:arglists (meta func))) | ||
| argmap (reduce | ||
| (fn [acc v] | ||
| (prn v) | ||
| (assoc acc (keyword v) v)) | ||
| ;; remove components and ctx | ||
| {} (drop-last 2 args)) | ||
| symkey (keyword (symbol (str *ns*) (str fsym)))] | ||
| #_(intern | ||
| *ns* fsym | ||
| (eval | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args))))) | ||
| (clojure.pprint/pprint | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args)))))) | ||
|
|
||
| (let [fsym 'delete-mock! | ||
| func #'back.api.controllers.mocks/delete-mock! | ||
| symkey (keyword (symbol (str *ns*) (str fsym))) | ||
| args (first (:arglists (meta func))) | ||
| argmap (reduce | ||
| (fn [acc v] | ||
| (cond | ||
| ;; Arg with a :as alias, making it not necessary to | ||
| ;; get the inner deconstructed keys. | ||
| (and (map? v) (some? (:as v))) | ||
| (assoc acc (keyword (:as v) (:as v))) | ||
|
|
||
| ;; Arg with only the :keys vec declared. | ||
| (and (map? v) (some? (:keys v))) | ||
| (reduce | ||
| (fn [acc arg] | ||
| (assoc acc (keyword arg) arg)) | ||
| acc (:keys v)) | ||
|
|
||
| ;; Normal arg types. | ||
| :else (assoc acc (keyword v) v))) | ||
| ;; remove components and ctx | ||
| {} (drop-last 2 args)) | ||
| callargs (map | ||
| (fn [arg] | ||
| (cond | ||
| (and (map? arg) (some? (:as arg))) | ||
| (:as arg) | ||
|
|
||
| (and (map? arg) (some? (:keys arg))) | ||
| (reduce | ||
| (fn [argmap arg] | ||
| ;; FIXME: not working for some reason | ||
| (prn :argmap argmap :argkey arg) | ||
| (assoc argmap (:keyword arg) arg)) | ||
| {} (:keys args)) | ||
|
|
||
| :else arg)) | ||
| args)] | ||
| (clojure.pprint/pprint | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@callargs))))) |
There was a problem hiding this comment.
Remove or complete the experimental logging wrapper code.
The experimental code at the end of the file has several issues:
- Contains debug statements (
prn) - Has commented out core functionality
- Contains unresolved FIXME
- Implements complex meta-programming that could be simplified
Consider either:
- Removing this experimental code until it's ready for production, or
- Completing it properly by:
- Removing debug statements
- Resolving the FIXME
- Uncommenting and testing the core functionality
- Adding proper documentation
If you'd like to keep this functionality, consider moving it to a separate namespace focused on logging utilities, such as back.api.utils.logging.
| (throw (ex-info "Mock with given wildcard and subdomain invalid" | ||
| {:status-code 412 | ||
| :cause :invalid-wildcard | ||
| :value (adapter.mocks/->wire ?existing-mock)})) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance error message with validation details.
The error message could be more helpful by including the specific validation failure details.
- (throw (ex-info "Mock with given wildcard and subdomain invalid"
+ (throw (ex-info (format "Mock with wildcard '%s' and subdomain '%s' already exists"
+ (:wildcard mock)
+ (:subdomain mock))
{:status-code 412
:cause :invalid-wildcard
- :value (adapter.mocks/->wire ?existing-mock)}))
+ :value (adapter.mocks/->wire ?existing-mock)
+ :conflict {:wildcard (:wildcard mock)
+ :subdomain (:subdomain mock)}}))Committable suggestion was skipped due to low confidence.
src/back/api/controllers/mocks.clj
Outdated
| (if-let [mock (-> (logic.mocks/unpack-domain domain) | ||
| (logic.mocks/->db-by-wildcard) | ||
| (db.mocks/get-mock-by-wildcard db ctx))] | ||
| (-> (logic.mocks/update-dns-status mock new-status) | ||
| (select-keys [:mock/id :mock/dns_status]) | ||
| (db.mocks/update! db ctx)) | ||
| (logs/trace | ||
| ::update-mock-dns-status! | ||
| {:mock-domain domain | ||
| :new-status new-status} | ||
| (-> (logic.mocks/update-dns-status mock new-status) | ||
| (select-keys [:mock/id :mock/dns_status]) | ||
| (db.mocks/update! db ctx))) | ||
| (throw (ex-info "No mock found with given domain" | ||
| {:cause :invalid-domain | ||
| :value domain})))) | ||
|
|
||
| (defn update-mock-unification-status! | ||
| [mock-id new-status db ctx] | ||
| (if-let [mock (db.mocks/get-mock-by-id (parse-uuid (str mock-id)) db ctx)] | ||
| (-> (logic.mocks/update-unification-status mock new-status) | ||
| (select-keys [:mock/id :mock/unification_status]) | ||
| (db.mocks/update! db ctx)) | ||
| (logs/trace | ||
| ::update-mock-unif-status! | ||
| {:mock-id mock-id | ||
| :new-status new-status} | ||
| (-> (logic.mocks/update-unification-status mock new-status) | ||
| (select-keys [:mock/id :mock/unification_status]) | ||
| (db.mocks/update! db ctx))) | ||
| (throw (ex-info "No mock found with given id" | ||
| {:status-code 400 | ||
| :cause :invalid-id | ||
| :value mock-id})))) | ||
|
|
||
| ;; Iterate over *ns* functions and replace them with themselves within | ||
| ;; a `logs/trace` call that uses each function's arglist as context. | ||
| (doseq [[fsym func] (ns-publics *ns*)] | ||
| (let [args (first (:arglists (meta func))) | ||
| argmap (reduce | ||
| (fn [acc v] | ||
| (prn v) | ||
| (assoc acc (keyword v) v)) | ||
| ;; remove components and ctx | ||
| {} (drop-last 2 args)) | ||
| symkey (keyword (symbol (str *ns*) (str fsym)))] | ||
| #_(intern | ||
| *ns* fsym | ||
| (eval | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args))))) | ||
| (clojure.pprint/pprint | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args)))))) | ||
|
|
||
| (let [fsym 'delete-mock! | ||
| func #'back.api.controllers.mocks/delete-mock! | ||
| symkey (keyword (symbol (str *ns*) (str fsym))) | ||
| args (first (:arglists (meta func))) | ||
| argmap (reduce | ||
| (fn [acc v] | ||
| (cond | ||
| ;; Arg with a :as alias, making it not necessary to | ||
| ;; get the inner deconstructed keys. | ||
| (and (map? v) (some? (:as v))) | ||
| (assoc acc (keyword (:as v) (:as v))) | ||
|
|
||
| ;; Arg with only the :keys vec declared. | ||
| (and (map? v) (some? (:keys v))) | ||
| (reduce | ||
| (fn [acc arg] | ||
| (assoc acc (keyword arg) arg)) | ||
| acc (:keys v)) | ||
|
|
||
| ;; Normal arg types. | ||
| :else (assoc acc (keyword v) v))) | ||
| ;; remove components and ctx | ||
| {} (drop-last 2 args)) | ||
| callargs (map | ||
| (fn [arg] | ||
| (cond | ||
| (and (map? arg) (some? (:as arg))) | ||
| (:as arg) | ||
|
|
||
| (and (map? arg) (some? (:keys arg))) | ||
| (reduce | ||
| (fn [argmap arg] | ||
| ;; FIXME: not working for some reason | ||
| (prn :argmap argmap :argkey arg) | ||
| (assoc argmap (:keyword arg) arg)) | ||
| {} (:keys args)) | ||
|
|
||
| :else arg)) | ||
| args)] | ||
| (clojure.pprint/pprint | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@callargs))))) |
There was a problem hiding this comment.
💡 Codebase verification
Migration to new logging component is consistent but incomplete
The verification reveals a mixed usage of logging patterns that needs attention:
- Most of the codebase has migrated to
com.moclojer.components.logswith the:as logsalias - Consistent usage of
logs/tracein theback/apimodules - However, there are still old-style logging calls in several files:
src/yaml_generator/ports/workers.cljusinglogs/logsrc/job_ops/controllers/mocks.cljusinglogs/logsrc/cloud_ops/api/controllers/*.cljusinglogs/log,logs/error,logs/warnsrc/dev/shadow/hooks.cljusing directs.util/log
🔗 Analysis chain
Verify complete migration to new logging component.
Let's verify the complete migration to the new logging component across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify migration to new logging component
# Check for any remaining old logging imports
echo "Checking for old logging imports..."
rg -l 'require.*\[(?!com\.moclojer\.components\.logs).*\.log[gs]\]' --type clojure
# Check for consistent usage of new logging namespace alias
echo "Checking for consistent logs alias usage..."
rg -l 'require.*\[com\.moclojer\.components\.logs :as (?!logs\])' --type clojure
# Check for any remaining direct logging calls
echo "Checking for old logging calls..."
rg '\b(?!logs/trace)\w+/(?:log|debug|info|warn|error)\b' --type clojure
Length of output: 1335
Script:
#!/bin/bash
# Description: Verify migration to new logging component with simpler patterns
# Check for any logging-related imports
echo "Checking logging imports..."
rg --pcre2 'require.*\.log[gs]' --type clojure
# Check for logging namespace aliases
echo "Checking logging namespace aliases..."
rg --pcre2 'require.*components\.logs :as' --type clojure
# Check for logging function calls
echo "Checking logging function calls..."
rg --pcre2 '\w+/(log|debug|info|warn|error|trace)\b' --type clojure -A 2
Length of output: 19066
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/back/api/controllers/mocks.clj(1 hunks)
🔇 Additional comments (1)
src/back/api/controllers/mocks.clj (1)
141-147: Consider adding optimistic locking for status updates.
The status update functions (update-mock-dns-status! and update-mock-unification-status!) could be susceptible to race conditions if multiple updates occur simultaneously. Consider implementing optimistic locking or version tracking.
Let's verify if there are any concurrent status updates in the codebase:
Also applies to: 155-161
src/back/api/controllers/mocks.clj
Outdated
| argmap (reduce | ||
| (fn [acc v] | ||
| (cond | ||
| ;; Arg with a :as alias, making it not necessary to | ||
| ;; get the inner deconstructed keys. | ||
| (and (map? v) (some? (:as v))) | ||
| (assoc acc (keyword (:as v) (:as v))) | ||
|
|
||
| ;; Arg with only the :keys vec declared. | ||
| (and (map? v) (some? (:keys v))) | ||
| (reduce | ||
| (fn [acc arg] | ||
| (assoc acc (keyword arg) arg)) | ||
| acc (:keys v)) | ||
|
|
||
| ;; Arg with :or declared (WIP) | ||
| (and (map? v) (some? (:or v))) | ||
| (reduce | ||
| (fn [v acc arg] | ||
| (assoc acc (keyword arg) | ||
| (get (:or v) arg))) | ||
| acc (:keys v)) | ||
|
|
||
| ;; Normal arg types. | ||
| :else (assoc acc (keyword v) v))) | ||
| ;; remove components and ctx |
There was a problem hiding this comment.
Simplify and secure the argument mapping logic.
The current argument mapping logic has several issues:
- Complex nested conditionals make it hard to maintain
- No validation of argument types before keyword conversion
- Potential security risk if user-controlled data is directly converted to keywords
Consider extracting this logic into a separate utility function with proper validation.
Here's a suggested approach:
(defn safe-arg->keyword [arg]
(cond
(nil? arg) nil
(keyword? arg) arg
(symbol? arg) (keyword (name arg))
:else (throw (ex-info "Invalid argument type for keyword conversion"
{:arg arg
:type (type arg)}))))
(defn build-arg-map [args]
(reduce
(fn [acc arg]
(cond
(map? arg)
(cond
(:as arg) (assoc acc (safe-arg->keyword (:as arg)) (:as arg))
(:keys arg) (merge acc
(into {}
(map (juxt safe-arg->keyword identity))
(:keys arg)))
:else acc)
:else (assoc acc (safe-arg->keyword arg) arg)))
{}
args))
src/back/api/controllers/mocks.clj
Outdated
| (clojure.pprint/pprint | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args)))))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove debug printing and consolidate example code.
The experimental code contains:
- Debug printing with
clojure.pprint - Redundant example for a single function
Either move this to a development/test namespace or remove it entirely before merging.
If you want to keep this as a development utility, consider moving it to:
src/dev/logging_utils.cljtest/back/api/controllers/mocks_test.clj
Also applies to: 234-237
src/back/api/controllers/mocks.clj
Outdated
| ;; Iterate over *ns* functions and replace them with themselves within | ||
| ;; a `logs/trace` call that uses each function's arglist as context. | ||
| (doseq [[fsym func] (ns-publics *ns*)] | ||
| (let [args (first (:arglists (meta func))) | ||
| argmap (reduce | ||
| (fn [acc v] | ||
| (prn v) | ||
| (assoc acc (keyword v) v)) | ||
| ;; remove components and ctx | ||
| {} (drop-last 2 args)) | ||
| symkey (keyword (symbol (str *ns*) (str fsym)))] | ||
| #_(intern | ||
| *ns* fsym | ||
| (eval | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args))))) | ||
| (clojure.pprint/pprint | ||
| `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@args)))))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Move experimental code to a development namespace.
The experimental logging wrapper code has several architectural concerns:
- Modifying namespace bindings at runtime is risky and can lead to unpredictable behavior
- The code is not production-ready (contains debug prints and commented code)
- The implementation belongs in a development utility namespace
Consider moving this code to a dedicated development namespace like dev.logging-utils and implementing it as a macro that can be used during development. This would:
- Keep production code clean
- Allow for proper testing of the logging wrapper
- Provide a safer way to experiment with logging enhancements
Example structure:
;; dev/logging_utils.clj
(ns dev.logging-utils
(:require [com.moclojer.components.logs :as logs]))
(defmacro wrap-ns-with-logging
"Development utility to wrap namespace functions with logging."
[ns-sym]
;; Implementation here
)| [id content {:keys [database mq]} ctx] | ||
| (if-let [mock (db.mocks/get-mock-by-id id database ctx)] | ||
| (let [updated-mock (-> mock | ||
| (logic.mocks/update {:content content}) | ||
| (db.mocks/update! database ctx) | ||
| (adapter.mocks/->wire)) | ||
| ->wired-old-mock (adapter.mocks/->wire mock)] | ||
| (logs/trace | ||
| ::update-mock! | ||
| {:mock-id id | ||
| :content-hash (hash content)} | ||
| (let [updated-mock (-> (logic.mocks/update mock {:content content}) | ||
| (db.mocks/update! database ctx) | ||
| (adapter.mocks/->wire)) | ||
| ->wired-old-mock (adapter.mocks/->wire mock)] | ||
|
|
||
| (ports.producers/generate-single-yml! (:id updated-mock) mq ctx) | ||
| (ports.producers/generate-single-yml! (:id updated-mock) mq ctx) | ||
|
|
||
| (when (and (= (:dns-status ->wired-old-mock) "offline") | ||
| (:enabled ->wired-old-mock)) | ||
| (ports.producers/create-domain! (logic.mocks/pack-domain ->wired-old-mock) | ||
| mq | ||
| ctx)) | ||
| (when (and (= (:dns-status ->wired-old-mock) "offline") | ||
| (:enabled ->wired-old-mock)) | ||
| (ports.producers/create-domain! | ||
| (logic.mocks/pack-domain ->wired-old-mock) mq ctx)) | ||
|
|
||
| updated-mock) | ||
| updated-mock)) | ||
| (throw (ex-info "Mock with given id invalid" | ||
| {:status-code 412 | ||
| :cause :invalid-id})))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add trace logging for error paths.
The error path in update-mock! lacks trace logging. Consider adding error context logging before throwing the exception.
(if-let [mock (db.mocks/get-mock-by-id id database ctx)]
(logs/trace
::update-mock!
{:mock-id id
:content-hash (hash content)}
(let [updated-mock (-> (logic.mocks/update mock {:content content})
(db.mocks/update! database ctx)
(adapter.mocks/->wire))
->wired-old-mock (adapter.mocks/->wire mock)]
(ports.producers/generate-single-yml! (:id updated-mock) mq ctx)
(when (and (= (:dns-status ->wired-old-mock) "offline")
(:enabled ->wired-old-mock))
(ports.producers/create-domain!
(logic.mocks/pack-domain ->wired-old-mock) mq ctx))
updated-mock))
- (throw (ex-info "Mock with given id invalid"
- {:status-code 412
- :cause :invalid-id}))))
+ (do
+ (logs/trace
+ ::update-mock-failed!
+ {:mock-id id
+ :reason :invalid-id})
+ (throw (ex-info "Mock with given id invalid"
+ {:status-code 412
+ :cause :invalid-id})))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [id content {:keys [database mq]} ctx] | |
| (if-let [mock (db.mocks/get-mock-by-id id database ctx)] | |
| (let [updated-mock (-> mock | |
| (logic.mocks/update {:content content}) | |
| (db.mocks/update! database ctx) | |
| (adapter.mocks/->wire)) | |
| ->wired-old-mock (adapter.mocks/->wire mock)] | |
| (logs/trace | |
| ::update-mock! | |
| {:mock-id id | |
| :content-hash (hash content)} | |
| (let [updated-mock (-> (logic.mocks/update mock {:content content}) | |
| (db.mocks/update! database ctx) | |
| (adapter.mocks/->wire)) | |
| ->wired-old-mock (adapter.mocks/->wire mock)] | |
| (ports.producers/generate-single-yml! (:id updated-mock) mq ctx) | |
| (ports.producers/generate-single-yml! (:id updated-mock) mq ctx) | |
| (when (and (= (:dns-status ->wired-old-mock) "offline") | |
| (:enabled ->wired-old-mock)) | |
| (ports.producers/create-domain! (logic.mocks/pack-domain ->wired-old-mock) | |
| mq | |
| ctx)) | |
| (when (and (= (:dns-status ->wired-old-mock) "offline") | |
| (:enabled ->wired-old-mock)) | |
| (ports.producers/create-domain! | |
| (logic.mocks/pack-domain ->wired-old-mock) mq ctx)) | |
| updated-mock) | |
| updated-mock)) | |
| (throw (ex-info "Mock with given id invalid" | |
| {:status-code 412 | |
| :cause :invalid-id})))) | |
| [id content {:keys [database mq]} ctx] | |
| (if-let [mock (db.mocks/get-mock-by-id id database ctx)] | |
| (logs/trace | |
| ::update-mock! | |
| {:mock-id id | |
| :content-hash (hash content)} | |
| (let [updated-mock (-> (logic.mocks/update mock {:content content}) | |
| (db.mocks/update! database ctx) | |
| (adapter.mocks/->wire)) | |
| ->wired-old-mock (adapter.mocks/->wire mock)] | |
| (ports.producers/generate-single-yml! (:id updated-mock) mq ctx) | |
| (when (and (= (:dns-status ->wired-old-mock) "offline") | |
| (:enabled ->wired-old-mock)) | |
| (ports.producers/create-domain! | |
| (logic.mocks/pack-domain ->wired-old-mock) mq ctx)) | |
| updated-mock)) | |
| (do | |
| (logs/trace | |
| ::update-mock-failed! | |
| {:mock-id id | |
| :reason :invalid-id}) | |
| (throw (ex-info "Mock with given id invalid" | |
| {:status-code 412 | |
| :cause :invalid-id}))))) |
src/back/api/controllers/mocks.clj
Outdated
| (doseq [[fsym func] (ns-publics *ns*)] | ||
| (let [symkey (keyword (symbol (str *ns*) (name fsym))) | ||
| args (first (:arglists (meta func))) | ||
| pre-argmap (fn [acc v] | ||
| (cond | ||
| ;; Arg with a :as alias, making it not necessary to | ||
| ;; get the inner deconstructed keys. | ||
| (and (map? v) (:as v)) | ||
| (acc (keyword (:as v)) (:as v)) | ||
| ;; Arg with only the :keys vec declared. | ||
| (and (map? v) (:keys v)) | ||
| (into {} (map (fn [k] | ||
| [(keyword k) k]) | ||
| (:keys v))) | ||
| ;; Normal arg types. | ||
| :else (assoc acc (keyword v) v))) | ||
| argmap (reduce pre-argmap {} (drop-last 2 args)) | ||
| callargs (map | ||
| (fn [arg] | ||
| (cond | ||
| (and (map? arg) (:as arg)) | ||
| (:as arg) | ||
| (and (map? arg) (:keys arg)) | ||
| (into {} (map (fn [a] | ||
| [(keyword a) a]) | ||
| (:keys arg))) | ||
| :else arg)) | ||
| args)] | ||
| (intern | ||
| *ns* fsym | ||
| (eval `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~symkey ~argmap | ||
| (~func ~@callargs))))))) |
There was a problem hiding this comment.
Function wrapping implementation could break function metadata and arity.
The current implementation has several technical issues:
- Wrapping functions loses their metadata (including doc strings and arity information)
- The wrapper doesn't preserve the function's variadic arguments
- Higher-order functions might not work correctly with wrapped functions
Consider using Clojure's alter-var-root with proper metadata preservation:
(defn wrap-with-logging [f sym]
(let [m (meta f)
wrapped (fn [& args]
(logs/trace sym
(zipmap (map keyword (first (:arglists m)))
args)
(apply f args)))]
(with-meta wrapped m)))
(doseq [[sym var] (ns-publics *ns*)]
(when (fn? @var)
(alter-var-root var #(wrap-with-logging % (keyword (str *ns*) (name sym))))))
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/back/api/controllers/mocks.clj(1 hunks)test/back/back/integration/api/wrap_traces_test.clj(1 hunks)
🔇 Additional comments (4)
test/back/back/integration/api/wrap_traces_test.clj (2)
1-11: LGTM: Namespace declaration and imports are well-organized
The namespace declaration follows Clojure conventions and includes all necessary dependencies for testing and component management.
38-42: 🧹 Nitpick (assertive)
Enhance test coverage and description
The current test implementation has several areas for improvement:
- The test description could be more specific about what "desired ns functions" means
- There's no error case testing
- The test might pass incorrectly if both
save-local-fnsandtrace-all-ns-testreturn empty lists
Let's verify if trace-all-ns-test is properly implemented:
Consider adding these test cases:
;; Test for non-empty results
(flow "ensures functions are actually being collected"
(match? (comp not empty?) (save-local-fns)))
;; Test for error cases
(flow "handles invalid namespaces gracefully"
(with-redefs [mocks/get-allowed-ns (constantly ['non.existent.ns])]
(match? empty? (save-local-fns))))src/back/api/controllers/mocks.clj (2)
12-15: LGTM! Consistent trace logging implementation.
The trace logging implementation is consistent across all functions and includes relevant contextual information.
Also applies to: 38-40, 48-51, 71-72, 80-82, 93-95, 110-113, 126-128, 142-145, 156-159
168-175: LGTM! Clear implementation of namespace filtering.
The function effectively filters namespaces to include only relevant ones for tracing.
| (defn- save-local-fns [] | ||
| (let [a-ns (mocks/get-allowed-ns) | ||
| m (atom [])] | ||
| (doseq [d-ns a-ns] | ||
| (doseq [[func _] (ns-publics d-ns)] | ||
| (swap! m conj (str func)))) | ||
| @m)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify save-local-fns implementation
The current implementation using an atom and nested doseq can be simplified using Clojure's sequence operations for better readability and performance.
Consider this more idiomatic implementation:
-(defn- save-local-fns []
- (let [a-ns (mocks/get-allowed-ns)
- m (atom [])]
- (doseq [d-ns a-ns]
- (doseq [[func _] (ns-publics d-ns)]
- (swap! m conj (str func))))
- @m))
+(defn- save-local-fns []
+ (->> (mocks/get-allowed-ns)
+ (mapcat ns-publics)
+ (map (comp str first))
+ (into [])))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn- save-local-fns [] | |
| (let [a-ns (mocks/get-allowed-ns) | |
| m (atom [])] | |
| (doseq [d-ns a-ns] | |
| (doseq [[func _] (ns-publics d-ns)] | |
| (swap! m conj (str func)))) | |
| @m)) | |
| (defn- save-local-fns [] | |
| (->> (mocks/get-allowed-ns) | |
| (mapcat ns-publics) | |
| (map (comp str first)) | |
| (into []))) |
| (defn- create-and-start-components [] | ||
| (component/start-system | ||
| (component/system-map | ||
| :config (components/new-config "back/config.edn") | ||
| :logger (component/using (components/new-logger) [:config]) | ||
| :http (components/new-http-mock []) | ||
| :router (components/new-router routes/routes) | ||
| :database (component/using (components/new-database) [:config]) | ||
| :mq (components/new-mq-mock) | ||
| :webserver (component/using (components/new-webserver) | ||
| [:config :http :router :database :mq])))) | ||
| (defn- save-local-fns [] | ||
| (let [a-ns (mocks/get-allowed-ns) | ||
| m (atom [])] | ||
| (doseq [d-ns a-ns] | ||
| (doseq [[func _] (ns-publics d-ns)] | ||
| (swap! m conj (str func)))) | ||
| @m)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add docstrings to helper functions
Both helper functions lack documentation. Please add docstrings explaining their purpose, arguments, and return values.
Example for create-and-start-components:
(defn- create-and-start-components []
+ "Creates and starts a new component system with all required dependencies.
+ Returns the started system map containing config, logger, http, router, database, mq, and webserver components."
(component/start-systemExample for save-local-fns:
(defn- save-local-fns []
+ "Retrieves all public function names from allowed namespaces.
+ Returns a vector of function names as strings."
(let [a-ns (mocks/get-allowed-ns)Committable suggestion skipped: line range outside the PR's diff.
| (if (seq ?existing-mock) | ||
| (throw (ex-info "Mock with given wildcard and subdomain invalid" | ||
| {:status-code 412 | ||
| :cause :invalid-wildcard | ||
| :value (adapter.mocks/->wire ?existing-mock)})) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error handling and add trace logging for error paths.
The error handling patterns are inconsistent:
- Some exceptions include
status-codewhile others don't - Error paths lack trace logging
Example implementation for standardized error handling:
- (throw (ex-info "Mock with given wildcard and subdomain invalid"
- {:status-code 412
- :cause :invalid-wildcard
- :value (adapter.mocks/->wire ?existing-mock)}))
+ (do
+ (logs/trace
+ ::create-mock-failed!
+ {:mock-id (:id mock)
+ :user-id user-id
+ :reason :invalid-wildcard})
+ (throw (ex-info "Mock with given wildcard and subdomain invalid"
+ {:status-code 412
+ :cause :invalid-wildcard
+ :value (adapter.mocks/->wire ?existing-mock)})))Also applies to: 63-67, 119-122, 132-135, 149-151, 163-166
src/back/api/controllers/mocks.clj
Outdated
| (defn inspect [a] (prn a) a) | ||
|
|
||
| (defn trace-all-ns-test [] | ||
| (let [a-ns (get-allowed-ns) | ||
| m (atom [])] | ||
| (doseq [curr-ns a-ns] | ||
| (doseq [[sym v] (ns-publics curr-ns)] | ||
| (when (fn? @v) | ||
| (swap! m conj (str (inspect ((meta v) :name)))) | ||
| (alter-var-root v #(wrap-with-trace % (keyword (str curr-ns) (name sym))))))) | ||
| @m)) |
There was a problem hiding this comment.
Remove debug code and move experimental implementation to dev namespace.
The current implementation contains:
- Debug printing with
prn - Experimental code mixed with production code
Consider:
- Removing debug statements
- Moving experimental code to a development namespace:
src/dev/logging_utils.cljtest/back/api/controllers/mocks_test.clj
src/back/api/controllers/mocks.clj
Outdated
| (defn trace-all-ns | ||
| "Iterate over *ns* functions and replace them with themselves within | ||
| a `logs/trace` call that uses each function's arglist as context." | ||
| [] | ||
| (let [desired-ns | ||
| (get-allowed-ns)] | ||
| (doseq [current-ns desired-ns] | ||
| (doseq [[fsym func] (ns-publics current-ns)] | ||
| (let [sym (keyword (symbol (str current-ns) (name fsym))) | ||
| m (meta func) | ||
| args (first (:arglists m)) | ||
| pre-argmap (fn [acc v] | ||
| (cond | ||
| ;; Arg with a :as alias, making it not necessary to | ||
| ;; get the inner deconstructed keys. | ||
| (and (map? v) (:as v)) | ||
| (acc (keyword (:as v)) (:as v)) | ||
| ;; Arg with only the :keys vec declared. | ||
| (and (map? v) (:keys v)) | ||
| (into {} (map (fn [k] | ||
| [(keyword k) k]) | ||
| (:keys v))) | ||
| ;; Normal arg types. | ||
| :else (assoc acc (keyword v) v))) | ||
| argmap (reduce pre-argmap {} (drop-last 2 args)) | ||
| callargs (map | ||
| (fn [arg] | ||
| (cond | ||
| (and (map? arg) (:as arg)) | ||
| (:as arg) | ||
| (and (map? arg) (:keys arg)) | ||
| (into {} (map (fn [a] [(keyword a) a]) (:keys arg))) | ||
| :else arg)) | ||
| args)] | ||
| (if-not (some #(= % '&) args) | ||
| (intern | ||
| *ns* fsym | ||
| (with-meta (eval `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~sym ~argmap | ||
| (~func ~@callargs)))) m)) | ||
| (logs/log "--> could not convert" func "args:" args))))))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the complex meta-programming implementation.
The current implementation:
- Has complex nested conditionals for argument mapping
- Might break function metadata
- Doesn't handle all edge cases
Consider simplifying using a more straightforward approach:
(defn safe-arg->keyword [arg]
(cond
(nil? arg) nil
(keyword? arg) arg
(symbol? arg) (keyword (name arg))
:else (throw (ex-info "Invalid argument type"
{:arg arg
:type (type arg)}))))
(defn build-arg-map [args]
(reduce
(fn [acc arg]
(cond
(map? arg)
(cond
(:as arg) (assoc acc (safe-arg->keyword (:as arg)) (:as arg))
(:keys arg) (merge acc
(into {}
(map (juxt safe-arg->keyword identity))
(:keys arg)))
:else acc)
:else (assoc acc (safe-arg->keyword arg) arg)))
{}
args))
(defn trace-all-ns []
(doseq [current-ns (get-allowed-ns)
[fsym func] (ns-publics current-ns)
:let [sym (keyword (str current-ns) (name fsym))
m (meta func)]]
(when-let [args (first (:arglists m))]
(when-not (some #{'&} args)
(intern *ns* fsym
(with-meta
(fn [& args]
(logs/trace sym
(build-arg-map args)
(apply func args)))
m))))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn trace-all-ns | |
| "Iterate over *ns* functions and replace them with themselves within | |
| a `logs/trace` call that uses each function's arglist as context." | |
| [] | |
| (let [desired-ns | |
| (get-allowed-ns)] | |
| (doseq [current-ns desired-ns] | |
| (doseq [[fsym func] (ns-publics current-ns)] | |
| (let [sym (keyword (symbol (str current-ns) (name fsym))) | |
| m (meta func) | |
| args (first (:arglists m)) | |
| pre-argmap (fn [acc v] | |
| (cond | |
| ;; Arg with a :as alias, making it not necessary to | |
| ;; get the inner deconstructed keys. | |
| (and (map? v) (:as v)) | |
| (acc (keyword (:as v)) (:as v)) | |
| ;; Arg with only the :keys vec declared. | |
| (and (map? v) (:keys v)) | |
| (into {} (map (fn [k] | |
| [(keyword k) k]) | |
| (:keys v))) | |
| ;; Normal arg types. | |
| :else (assoc acc (keyword v) v))) | |
| argmap (reduce pre-argmap {} (drop-last 2 args)) | |
| callargs (map | |
| (fn [arg] | |
| (cond | |
| (and (map? arg) (:as arg)) | |
| (:as arg) | |
| (and (map? arg) (:keys arg)) | |
| (into {} (map (fn [a] [(keyword a) a]) (:keys arg))) | |
| :else arg)) | |
| args)] | |
| (if-not (some #(= % '&) args) | |
| (intern | |
| *ns* fsym | |
| (with-meta (eval `(fn [~@args] | |
| (com.moclojer.components.logs/trace ~sym ~argmap | |
| (~func ~@callargs)))) m)) | |
| (logs/log "--> could not convert" func "args:" args))))))) | |
| (defn safe-arg->keyword [arg] | |
| (cond | |
| (nil? arg) nil | |
| (keyword? arg) arg | |
| (symbol? arg) (keyword (name arg)) | |
| :else (throw (ex-info "Invalid argument type" | |
| {:arg arg | |
| :type (type arg)})))) | |
| (defn build-arg-map [args] | |
| (reduce | |
| (fn [acc arg] | |
| (cond | |
| (map? arg) | |
| (cond | |
| (:as arg) (assoc acc (safe-arg->keyword (:as arg)) (:as arg)) | |
| (:keys arg) (merge acc | |
| (into {} | |
| (map (juxt safe-arg->keyword identity)) | |
| (:keys arg))) | |
| :else acc) | |
| :else (assoc acc (safe-arg->keyword arg) arg))) | |
| {} | |
| args)) | |
| (defn trace-all-ns [] | |
| (doseq [current-ns (get-allowed-ns) | |
| [fsym func] (ns-publics current-ns) | |
| :let [sym (keyword (str current-ns) (name fsym)) | |
| m (meta func)]] | |
| (when-let [args (first (:arglists m))] | |
| (when-not (some #{'&} args) | |
| (intern *ns* fsym | |
| (with-meta | |
| (fn [& args] | |
| (logs/trace sym | |
| (build-arg-map args) | |
| (apply func args))) | |
| m)))))) |
|
@J0sueTM como estamos com esse PR? |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range comments (2)
src/cloud_ops/api/server.clj (2)
Line range hint
23-26: Remove legacy logger setup after migrationThe
start-system!function still uses the old logging setup viacomponents/setup-logger. This should be removed once the new logger component is properly integrated into the system map.Apply this change:
(defn start-system! [system-map] - (components/setup-logger [["*" :info]] :auto :prod) (->> system-map component/start (reset! system-atom)))
Line range hint
28-31: Update logging call to use new logger componentThe
stop-system!function uses the old logging namespace directly. This should be updated to use the new logger component.Consider updating to use the new logger component:
(defn stop-system! [] - (logs/log :info "stopping system") + (when-let [logger (:logger @system-atom)] + (.info logger "stopping system")) (swap! system-atom (fn [s] (when s (component/stop s)))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
resources/back/config.edn(1 hunks)src/back/api/adapters/mocks.clj(1 hunks)src/back/api/controllers/mocks.clj(1 hunks)src/back/api/server.clj(1 hunks)src/back/api/utils.clj(1 hunks)src/cloud_ops/api/server.clj(1 hunks)src/dev/api/dev.clj(4 hunks)src/dev/utils.clj(2 hunks)
🔇 Additional comments (16)
src/back/api/utils.clj (1)
Line range hint 3-9: LGTM! Verify usage in mock transformations.
The implementation is clean and follows functional programming principles. The function is pure and its purpose is well-documented.
Let's verify its usage in mock transformations:
✅ Verification successful
Function is correctly used in mock data transformations
The verification confirms that assoc-if is properly used in mock transformations:
- Used in
src/back/api/adapters/mocks.cljwhich correctly imports it fromback.api.utils - Applied in the
->wirefunction to conditionally set:org-idand:contentfields - Same pattern is used in
src/job_ops/adapters/mocks.clj(likely a duplicate that should be consolidated)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of assoc-if in mock-related code
# Expected: Usage in ->wire function for mock transformations
# Search for assoc-if usage in mock-related files
rg -A 5 "assoc-if" --glob "**/mock*.clj"
# Search for ->wire function implementation to verify transformation logic
ast-grep --pattern '(defn ->wire [$$$] $$$)'
Length of output: 993
src/back/api/adapters/mocks.clj (2)
1-3: LGTM! Clean namespace formatting.
The multi-line style improves readability while maintaining the same functionality.
Line range hint 1-15: Verify if logging should be added to this adapter.
Given that this PR's objective is to update logging with Telemere, and this file handles mock data transformation, consider if we should add logging for important operations (e.g., failed transformations, invalid input).
src/dev/api/dev.clj (2)
44-45: Enhance logger usage example with actual logging calls
While the example shows how to access the logger component, it would be more helpful to demonstrate actual logging usage with Telemere.
18-18: LGTM: Logger component integration looks correct
The logger component is properly integrated with a dependency on the config component, which aligns with the PR objective of updating to the new Telemere-based logging system.
Let's verify the logger component configuration:
✅ Verification successful
Logger component integration is consistent across the codebase
The logger component is correctly configured in src/dev/api/dev.clj and matches the same pattern used in other system maps (src/back/api/server.clj and test files). All instances properly use component/using with a dependency on :config.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the logger component is consistently configured across the codebase
ast-grep --pattern 'components/new-logger'
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search for logger component configuration
rg -l "new-logger|components.*logger" --type clojure
# Also search for any logger-related component definitions
ast-grep --pattern '(defn new-logger'
# Check for any logger component usage in system maps
rg "(:logger|logger:)" --type clojure -B 2 -A 2
Length of output: 1790
src/back/api/server.clj (3)
2-9: LGTM: Clean namespace organization
The new requires for logging components are properly organized and follow Clojure naming conventions.
Line range hint 16-33: Consider explicit Telemere configuration
The system map now includes the logger component, but there's no explicit configuration for Telemere integration mentioned in the PR objectives. Consider adding Telemere-specific configuration in the :config component.
17-17: Verify component dependencies on logger
The logger component is correctly added with config dependency. However, other components might need to declare dependency on the logger to ensure proper initialization order.
resources/back/config.edn (3)
32-32: Review the index naming convention for production use
The index name includes "test" which suggests it's not intended for production use.
28-32: Verify OpenSearch configuration with Telemere integration
Since this PR is about integrating with Telemere for logging, please ensure the OpenSearch configuration aligns with Telemere's requirements.
✅ Verification successful
Let me gather more information about the logging configuration and Telemere usage.
OpenSearch configuration appears correct for Telemere integration
The OpenSearch configuration aligns with Telemere's requirements as evidenced by:
- The codebase shows active usage of Telemere (
taoensso.telemere) for logging across multiple components - Extensive trace logging is implemented using the logging framework
- The OpenSearch configuration provides all necessary connection parameters (host, port, credentials, index) for log aggregation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for Telemere configuration or usage patterns
rg -A 5 "telemere|Telemere" --type clojure
# Check for any logging configuration patterns
ast-grep --pattern 'logs/trace $$$'
Length of output: 867
Script:
#!/bin/bash
# Check for Telemere configuration patterns
rg -A 10 "taoensso.telemere" --type clojure
# Look for logging configuration in config files
rg -A 5 "opensearch|logging|telemere" resources/
# Check for any trace or logging function usage
rg -A 3 "logs/trace|logs/info|logs/debug" --type clojure
Length of output: 11573
28-32: 🛠️ Refactor suggestion
Add environment-specific configuration for OpenSearch
Other sections in the file use #profile for environment-specific settings, but the OpenSearch configuration doesn't follow this pattern. Consider aligning with the existing structure:
:opensearch {:username #or [#env OPENSEARCH_USERNAME "foobar"]
:password #or [#env OPENSEARCH_PASSWORD "foobar"]
- :host #or [#env OPENSEARCH_HOST "foobar.com"]
- :port #or [#env OPENSEARCH_PORT 25060]
+ :host #profile {:dev #or [#env OPENSEARCH_HOST "localhost"]
+ :prod #or [#env OPENSEARCH_HOST "opensearch.production.com"]}
+ :port #profile {:dev #or [#env OPENSEARCH_PORT 9200]
+ :prod #or [#env OPENSEARCH_PORT 25060]}
:index #or [#env OPENSEARCH_INDEX "moclojer-api-test-tracing"]}Likely invalid or redundant comment.
src/dev/utils.clj (2)
11-13:
Fix system startup trace implementation
The call to utils/trace-all-ns will fail due to the circular dependency. Additionally, it's called after the function returns from the 3-arity version, which is incorrect.
Apply this fix:
(defn start-system-dev!
([sys-atom sys-map]
- (start-system-dev! sys-atom sys-map true)
- (utils/trace-all-ns))
+ (start-system-dev! sys-atom sys-map true)
+ (trace-all-ns))Likely invalid or redundant comment.
41-81: 🧹 Nitpick (assertive)
Consider performance implications and simplify argument handling
The tracing implementation has several considerations:
- Wrapping all public functions with tracing will impact performance
- The argument handling logic is complex and could be simplified
- There's no way to disable tracing once enabled
Consider these improvements:
- Add a way to selectively enable/disable tracing per namespace
- Add a function to restore original implementations
- Simplify the argument handling using a helper function
Let's check the impact on the codebase:
Consider splitting the complex argument handling into a separate function:
(defn- prepare-args [args]
(let [process-arg (fn [arg]
(cond
(and (map? arg) (:as arg)) (:as arg)
(and (map? arg) (:keys arg)) (into {} (map (fn [a] [(keyword a) a]) (:keys arg)))
:else arg))]
(map process-arg args)))src/back/api/controllers/mocks.clj (3)
2-8: LGTM! Imports are correctly updated for the new logging system.
The namespace declaration properly imports the new logging component as expected.
12-15: Consider adding trace logging for error paths.
While the success path is well logged, adding trace logging before throwing exceptions would improve debugging capabilities.
7-7: Verify complete migration to new logging component.
The migration to the new logging component looks complete in this file. Let's verify the entire codebase.
Also applies to: 12-15, 38-41, 48-51, 71-72, 80-83, 93-95, 110-113, 126-128, 142-145, 156-159
✅ Verification successful
Migration to new logging component is complete
Based on the verification results:
- All logging imports consistently use
[com.moclojer.components.logs :as logs] - All logging calls use the new
logs/logfunction with the correct namespace - No old or inconsistent logging patterns were found
- The only other logging-related imports found are for unrelated logic modules (digital-ocean, cloudflare)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify migration to new logging component across the codebase
# Check for any remaining old logging imports
echo "Checking for old logging imports..."
rg -l 'require.*\[(?!com\.moclojer\.components\.logs).*\.log[gs]\]' --type clojure
# Check for consistent usage of new logging namespace alias
echo "Checking for consistent logs alias usage..."
rg -l 'require.*\[com\.moclojer\.components\.logs :as (?!logs\])' --type clojure
# Check for any remaining direct logging calls
echo "Checking for old logging calls..."
rg '\b(?!logs/trace)\w+/(?:log|debug|info|warn|error)\b' --type clojure
Length of output: 1335
Script:
#!/bin/bash
# Description: Verify migration to new logging component across the codebase
# Check for any remaining old logging imports
echo "Checking for old logging imports..."
rg --pcre2 'require.*\[(?!com\.moclojer\.components\.logs).*\.log[gs]\]' --type clojure
# Check for consistent usage of new logging namespace alias
echo "Checking for consistent logs alias usage..."
rg --pcre2 'require.*\[com\.moclojer\.components\.logs :as (?!logs\])' --type clojure
# Check for any remaining direct logging calls
echo "Checking for old logging calls..."
rg --pcre2 '\b(?!logs/trace)\w+/(?:log|debug|info|warn|error)\b' --type clojure
# Also check for any logging-related imports and usages more broadly
echo "Checking for any logging-related imports..."
rg 'require.*log' --type clojure
echo "Checking for any logging function calls..."
rg '\w+/(log|debug|info|warn|error)' --type clojure
Length of output: 12577
| [com.moclojer.components.core :as components] | ||
| [com.moclojer.components.logs :as logs] | ||
| [com.moclojer.components.mq :as mq] | ||
| [dev.utils :refer [trace-all-ns]] |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused trace-all-ns import
The trace-all-ns function from dev.utils is imported but never used in both server files. This development utility is only used in development-specific code paths (as seen in src/dev/utils.clj). Remove this unused import to keep the production code clean.
- Remove
[dev.utils :refer [trace-all-ns]]fromsrc/cloud_ops/api/server.clj - Remove
[dev.utils :refer [trace-all-ns]]fromsrc/back/api/server.clj
🔗 Analysis chain
Verify the necessity of trace-all-ns in production code
The dev.utils namespace with trace-all-ns appears to be a development utility. Consider:
- Moving this import to a development-only configuration
- Using conditional compilation
#+clj-kondo/ignoreif needed for development
Let me check the actual usage of trace-all-ns in the server files to understand if it's actually being used in production code.
Let me check how this utility is used in the actual server initialization to make a final determination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if dev.utils is used in development files
rg -l "dev\.utils" | grep -i "dev\|test"
# Check for other occurrences of trace-all-ns
rg "trace-all-ns"
Length of output: 465
Script:
#!/bin/bash
# Check the content of server files to see how trace-all-ns is used
ast-grep --pattern 'trace-all-ns'
# Also check the content of dev.utils to understand its purpose
cat src/dev/utils.clj
Length of output: 3084
Script:
#!/bin/bash
# Check the content of both server files to see the context of trace-all-ns import
cat src/cloud_ops/api/server.clj
cat src/back/api/server.clj
Length of output: 3542
| :opensearch {:username #or [#env OPENSEARCH_USERNAME "foobar"] | ||
| :password #or [#env OPENSEARCH_PASSWORD "foobar"] | ||
| :host #or [#env OPENSEARCH_HOST "foobar.com"] | ||
| :port #or [#env OPENSEARCH_PORT 25060] | ||
| :index #or [#env OPENSEARCH_INDEX "moclojer-api-test-tracing"]} |
There was a problem hiding this comment.
💡 Codebase verification
Default credentials in configuration require attention
The verification reveals that "foobar" is consistently used as a default value for multiple sensitive configuration settings:
- Sentry authentication token
- OpenSearch credentials (username, password, host)
- Digital Ocean token
- Cloudflare token
While other occurrences in test files and example code are legitimate test data, having "foobar" as default credentials in the main configuration file is problematic because:
- It's used across multiple critical authentication settings
- These defaults could accidentally be used in non-development environments
- The pattern of using the same value ("foobar") for multiple credentials reduces security
Recommendations:
- Use different, more secure default values for each credential
- Add validation to ensure these defaults cannot be used in production
- Consider using placeholder values like
REPLACE_MEthat would fail obviously if not configured - Add comments indicating these are development-only values
🔗 Analysis chain
Security: Review default credentials for OpenSearch configuration
The default credentials ("foobar") are insecure and could pose a security risk if accidentally used in production. Consider:
- Using more secure default values for local development
- Adding validation to prevent these defaults in production
- Adding comments to indicate these are development-only values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these default credentials are used in any other configuration files
rg -l "foobar" --type-add 'config:*.{edn,clj,yml,yaml,json}' -t config
Length of output: 275
Script:
#!/bin/bash
# Let's check the context of "foobar" usage in these files
echo "=== config.edn ==="
rg "foobar" resources/back/config.edn -C 2
echo -e "\n=== yml.clj ==="
rg "foobar" src/yaml_generator/logic/yml.clj -C 2
echo -e "\n=== digital_ocean.clj ==="
rg "foobar" src/cloud_ops/api/logic/digital_ocean.clj -C 2
echo -e "\n=== Test files ==="
rg "foobar" test/back/back/integration/api/*.clj -C 2
Length of output: 3372
src/dev/utils.clj
Outdated
| (:require | ||
| [clojure.string :as str] | ||
| [com.moclojer.components.logs :as logs] | ||
| [com.moclojer.components.migrations :as migrations] | ||
| [com.stuartsierra.component :as component] | ||
| [dev.utils :as utils] | ||
| [pg-embedded-clj.core :as pg-emb])) |
There was a problem hiding this comment.
Remove circular dependency in namespace declarations
The namespace is requiring itself through [dev.utils :as utils], which creates a circular dependency. This can lead to initialization issues and is generally considered an anti-pattern in Clojure.
Remove the self-reference:
- [dev.utils :as utils]And update any utils/ calls to use local function references.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (:require | |
| [clojure.string :as str] | |
| [com.moclojer.components.logs :as logs] | |
| [com.moclojer.components.migrations :as migrations] | |
| [com.stuartsierra.component :as component] | |
| [dev.utils :as utils] | |
| [pg-embedded-clj.core :as pg-emb])) | |
| (:require | |
| [clojure.string :as str] | |
| [com.moclojer.components.logs :as logs] | |
| [com.moclojer.components.migrations :as migrations] | |
| [com.stuartsierra.component :as component] | |
| [pg-embedded-clj.core :as pg-emb])) |
src/dev/utils.clj
Outdated
| (defn- get-allowed-ns [] | ||
| (->> (all-ns) | ||
| (map ns-name) | ||
| (map name) | ||
| (filter #(and (str/starts-with? % "com.moclojer") | ||
| (not (str/ends-with? % "logs")))) | ||
| (map symbol) | ||
| (into []))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add docstring to private function
The get-allowed-ns function would benefit from a docstring explaining its filtering logic and return value.
(defn- get-allowed-ns []
+ "Returns a vector of symbols representing allowed namespaces.
+ Includes namespaces starting with 'com.moclojer' but excludes those ending with 'logs'
+ to prevent recursive tracing."
(->> (all-ns)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn- get-allowed-ns [] | |
| (->> (all-ns) | |
| (map ns-name) | |
| (map name) | |
| (filter #(and (str/starts-with? % "com.moclojer") | |
| (not (str/ends-with? % "logs")))) | |
| (map symbol) | |
| (into []))) | |
| (defn- get-allowed-ns [] | |
| "Returns a vector of symbols representing allowed namespaces. | |
| Includes namespaces starting with 'com.moclojer' but excludes those ending with 'logs' | |
| to prevent recursive tracing." | |
| (->> (all-ns) | |
| (map ns-name) | |
| (map name) | |
| (filter #(and (str/starts-with? % "com.moclojer") | |
| (not (str/ends-with? % "logs")))) | |
| (map symbol) | |
| (into []))) |
| (when (and (= (:dns-status ->wired-old-mock) "offline") | ||
| (:enabled ->wired-old-mock)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using keywords for status values.
Status values like "offline" should be keywords (:offline) for consistency and to prevent string comparison issues. This applies to all status-related operations.
- (= (:dns-status ->wired-old-mock) "offline")
+ (= (:dns-status ->wired-old-mock) :offline)Also applies to: 142-145, 156-159
| (->> (db.mocks/get-mocks (parse-uuid (str uuid)) database ctx) | ||
| (map adapter.mocks/->wire)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding performance logging for database operations.
Database operations and external service calls (like YAML generation) would benefit from performance tracking. Consider wrapping these operations with timing logs.
Example implementation:
(defn with-timing [op-name f]
(let [start (System/nanoTime)
result (f)
duration (/ (- (System/nanoTime) start) 1e6)]
(logs/trace ::operation-timing
{:operation op-name
:duration_ms duration})
result))
;; Usage in get-mocks:
(with-timing :fetch-mocks
#(db.mocks/get-mocks (parse-uuid (str uuid)) database ctx))Also applies to: 83-88, 96-101, 115-117
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
src/back/api/server.clj(2 hunks)src/back/api/utils.clj(1 hunks)src/dev/shadow/hooks.clj(1 hunks)src/dev/utils.clj(1 hunks)
🔇 Additional comments (6)
src/back/api/utils.clj (2)
3-9: LGTM!
The function is well-documented and follows Clojure's functional programming principles. The implementation is correct and efficient.
11-17: 🛠️ Refactor suggestion
Consider using Telemere logging component instead of prn
Given that this PR's objective is to update logging to use Telemere, the inspect function should leverage the new logging infrastructure rather than using prn directly. This would provide better consistency, structured logging, and proper log levels.
Additionally:
- Consider renaming to
spyor usingtap>to align with common Clojure debugging patterns - The docstring should explicitly mention the printing side effect
Here's a suggested improvement (assuming Telemere logger is required as log):
-(defn inspect
- "Inspects a variable's contents and returns it without modifying its value."
+;; Consider using tap> or spy naming
+(defn inspect
+ "Inspects a variable's contents by logging it at debug level and returns
+ the original value without modification."
[v]
(if (instance? clojure.lang.Atom v)
- (prn @v)
- (prn v))
+ (log/debug "Inspecting atom:" @v)
+ (log/debug "Inspecting value:" v))
v)Let's verify the Telemere logger usage in the codebase:
src/dev/utils.clj (3)
8-8: Remove circular dependency in namespace declarations
The namespace is requiring itself through [dev.utils :as utils], which creates a circular dependency and can lead to initialization issues.
Apply this diff to remove the self-reference:
- [dev.utils :as utils]Update any utils/ calls to use local function references.
11-18: Add docstring to private function get-allowed-ns
Adding a docstring would enhance code readability and maintainability by explaining the function's purpose and behavior.
Apply this diff to add a docstring:
(defn get-allowed-ns []
+ "Returns a vector of symbols representing allowed namespaces.
+ Includes namespaces starting with 'com.moclojer' but excludes those ending with 'logs'
+ to prevent recursive tracing."
(->> (all-ns)84-85:
Fix circular dependency by removing self-reference and updating function call
The call to utils/trace-all-ns is causing a circular dependency due to the namespace requiring itself.
Apply this diff to remove the self-reference and update the function call:
- (utils/trace-all-ns))
+ (trace-all-ns))This removes the dependency on utils and calls trace-all-ns directly since it's defined in the same namespace.
Likely invalid or redundant comment.
src/back/api/server.clj (1)
17-17: LGTM!
The :logger component is correctly added to the system map and properly wired with its dependencies.
src/dev/utils.clj
Outdated
| (defn trace-all-ns-1 | ||
| "Iterate over *ns* functions and replace them with themselves within | ||
| a `logs/trace` call that uses each function's arglist as context." | ||
| [] | ||
| (let [desired-ns | ||
| (get-allowed-ns)] | ||
| (doseq [current-ns desired-ns] | ||
| (doseq [[fsym func] (ns-publics current-ns)] | ||
| (let [sym (keyword (symbol (str current-ns) (name fsym))) | ||
| m (meta func) | ||
| args (first (:arglists m)) | ||
| pre-argmap (fn [acc v] | ||
| (cond | ||
| ;; Arg with a :as alias, making it not necessary to | ||
| ;; get the inner deconstructed keys. | ||
| (and (map? v) (:as v)) | ||
| (acc (keyword (:as v)) (:as v)) | ||
| ;; Arg with only the :keys vec declared. | ||
| (and (map? v) (:keys v)) | ||
| (into {} (map (fn [k] | ||
| [(keyword k) k]) | ||
| (:keys v))) | ||
| ;; Normal arg types. | ||
| :else (assoc acc (keyword v) v))) | ||
| argmap (reduce pre-argmap {} (drop-last 2 args)) | ||
| callargs (map | ||
| (fn [arg] | ||
| (cond | ||
| (and (map? arg) (:as arg)) | ||
| (:as arg) | ||
| (and (map? arg) (:keys arg)) | ||
| (into {} (map (fn [a] [(keyword a) a]) (:keys arg))) | ||
| :else arg)) | ||
| args)] | ||
| (if-not (some #(= % '&) args) | ||
| (intern | ||
| *ns* fsym | ||
| (with-meta (eval `(fn [~@args] | ||
| (com.moclojer.components.logs/trace ~sym ~argmap | ||
| (~func ~@callargs)))) m)) | ||
| (prn "--> could not convert" func "args:" args))))))) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing or documenting the unused function trace-all-ns-1
The function trace-all-ns-1 is defined but not used anywhere in the codebase. Keeping unused code can lead to confusion and increase maintenance overhead.
If this function is not needed, consider removing it to keep the codebase clean. Alternatively, if it's intended for future use, add a comment explaining its purpose and why it's currently unused.
src/back/api/server.clj
Outdated
| [com.moclojer.components.logs :as logs] | ||
| [com.moclojer.components.migrations :as migrations] | ||
| [com.stuartsierra.component :as component] | ||
| [dev.utils :refer [trace-all-ns]]) |
There was a problem hiding this comment.
Avoid using development utilities in production code
Importing dev.utils in production code can lead to unintended side effects and potential security risks.
Consider removing the reference to dev.utils and ensure that production code does not depend on development-only namespaces.
If tracing is required in production, refactor the necessary functions into a production-appropriate namespace.
src/back/api/server.clj
Outdated
| (reset! system-atom)) | ||
| (trace-all-ns)) |
There was a problem hiding this comment.
Remove call to trace-all-ns in production code
The trace-all-ns function is intended for development and may impact performance or reveal sensitive information when used in production.
Recommend removing the call to trace-all-ns from the start-system! function.
Apply this diff to remove the call:
(->> system-map
component/start
(reset! system-atom))
- (trace-all-ns))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (reset! system-atom)) | |
| (trace-all-ns)) | |
| (reset! system-atom)) |
| (do | ||
| (s.util/log {:type ::source-does-not-exist | ||
| :source source}) | ||
| (constantly build-state)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve readability by refining the do block formatting
The consolidation of the do block enhances readability by maintaining consistent indentation and grouping related actions.
No changes to functionality are introduced, and the code remains functionally equivalent.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/back/api/utils.clj(1 hunks)src/dev/utils.clj(2 hunks)
🔇 Additional comments (4)
src/back/api/utils.clj (1)
3-9: LGTM! Well-documented utility function
The implementation is correct and the docstring clearly explains its purpose.
src/dev/utils.clj (3)
91-92: Verify the effects of calling trace-all-ns in start-system-dev!
The call to trace-all-ns has been added to start-system-dev!. Ensure that invoking trace-all-ns during system startup does not introduce unintended side effects or performance issues, especially in non-development environments.
10-18: 🧹 Nitpick (assertive)
Add docstring to get-allowed-ns function
The function get-allowed-ns lacks a docstring explaining its purpose and behavior. Adding a docstring will improve code readability and maintainability.
19-60: 🧹 Nitpick (assertive)
Consider removing or documenting the unused trace-all-ns-1 function
The trace-all-ns-1 function is defined but not used anywhere in the codebase. Unused code can lead to confusion and increase maintenance overhead.
If this function is intended for future use, consider adding a comment explaining its purpose. Otherwise, consider removing it to keep the codebase clean.
src/back/api/utils.clj
Outdated
| (defn inspect | ||
| "Inspects a variable's contents and returns it without modifying its value." | ||
| [v & a] | ||
| (if (instance? clojure.lang.Atom v) | ||
| (prn "Deref atom: " @v) | ||
| (prn v)) | ||
| (if a | ||
| (let [c (count a)] | ||
| (dotimes [i c] | ||
| (if (instance? clojure.lang.Atom a) | ||
| (prn (nth @a i)) | ||
| (prn (nth a i))))) | ||
| v)) |
There was a problem hiding this comment.
Critical issues in the inspect function implementation
Several issues need to be addressed:
- Bug: Line 20 incorrectly checks if
a(the sequence) is an atom, instead of checking individual elements - The function's docstring should clarify the return value behavior
- Consider using the new Telemere logging system instead of
prnfor consistency with PR objectives
Here's the suggested fix:
(defn inspect
- "Inspects a variable's contents and returns it without modifying its value."
+ "Inspects a variable's contents and any additional arguments, printing them to the log.
+ Returns the inspected value v. For atoms, prints and returns the dereferenced value."
[v & a]
- (if (instance? clojure.lang.Atom v)
- (prn "Deref atom: " @v)
- (prn v))
- (if a
- (let [c (count a)]
- (dotimes [i c]
- (if (instance? clojure.lang.Atom a)
- (prn (nth @a i))
- (prn (nth a i)))))
- v))
+ (let [log-value (if (instance? clojure.lang.Atom v) @v v)]
+ (logs/trace "Inspect value:" log-value)
+ (when a
+ (doseq [arg a]
+ (logs/trace "Additional arg:"
+ (if (instance? clojure.lang.Atom arg) @arg arg))))
+ log-value))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn inspect | |
| "Inspects a variable's contents and returns it without modifying its value." | |
| [v & a] | |
| (if (instance? clojure.lang.Atom v) | |
| (prn "Deref atom: " @v) | |
| (prn v)) | |
| (if a | |
| (let [c (count a)] | |
| (dotimes [i c] | |
| (if (instance? clojure.lang.Atom a) | |
| (prn (nth @a i)) | |
| (prn (nth a i))))) | |
| v)) | |
| (defn inspect | |
| "Inspects a variable's contents and any additional arguments, printing them to the log. | |
| Returns the inspected value v. For atoms, prints and returns the dereferenced value." | |
| [v & a] | |
| (let [log-value (if (instance? clojure.lang.Atom v) @v v)] | |
| (logs/trace "Inspect value:" log-value) | |
| (when a | |
| (doseq [arg a] | |
| (logs/trace "Additional arg:" | |
| (if (instance? clojure.lang.Atom arg) @arg arg)))) | |
| log-value)) |
src/back/api/utils.clj
Outdated
| (ns back.api.utils) | ||
|
|
||
| (defn assoc-if | ||
| (defn assoc-if | ||
| "This function returns a map with a respective associated key and value | ||
| if it is given as argument." | ||
| [m k v] | ||
| (if v | ||
| (assoc m k v) | ||
| m)) | ||
|
|
||
| (defn inspect | ||
| "Inspects a variable's contents and returns it without modifying its value." | ||
| [v & a] | ||
| (if (instance? clojure.lang.Atom v) | ||
| (prn "Deref atom: " @v) | ||
| (prn v)) | ||
| (if a | ||
| (let [c (count a)] | ||
| (dotimes [i c] | ||
| (if (instance? clojure.lang.Atom a) | ||
| (prn (nth @a i)) | ||
| (prn (nth a i))))) | ||
| v)) | ||
|
|
||
| (defn inspect-if | ||
| "Inspects if condition is met" | ||
| [c v & a] | ||
| (when c | ||
| (inspect v (when a a)))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Codebase verification
Debugging utilities should use structured logging via com.moclojer.components.logs
The codebase consistently uses structured logging through com.moclojer.components.logs with logs/trace for detailed debugging. The current implementation using prn should be updated to:
- Add the required import:
[com.moclojer.components.logs :as logs]- Replace direct
prncalls with structured logging:
- In
inspect: Uselogs/trace ::inspectwith structured data - In
inspect-if: Uselogs/trace ::inspect-ifwith structured data
This aligns with the established logging patterns seen across the codebase, particularly in controllers and DB layers.
🔗 Analysis chain
Verify integration with Telemere logging system
While these utility functions are helpful for debugging, they should be integrated with the new Telemere logging system mentioned in the PR objectives, rather than using prn directly.
Let's check how Telemere logging is used in other files:
Consider creating a dedicated logging namespace that wraps Telemere functionality and using it consistently across these utility functions.
Let's examine how Telemere logging is actually used in these files to understand the correct pattern.
Let's check the required imports and logging setup to understand the complete pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Telemere logging patterns to understand the correct usage
rg -l "telemere|logs/trace|logs/log" --type clojure
Length of output: 717
Script:
#!/bin/bash
# Look for actual Telemere logging usage patterns in a few key files
rg "telemere|logs/trace|logs/log" --type clojure -A 2 src/back/api/
Length of output: 6982
Script:
#!/bin/bash
# Check the logging setup and imports in a few example files
rg "^\s*\[.*logs.*\]|^\s*\[.*telemere.*\]" --type clojure -A 2 src/back/api/
Length of output: 1645
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/back/api/utils.clj(1 hunks)src/dev/utils.clj(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (2)
src/back/api/utils.clj (2)
1-4: LGTM: Namespace declaration aligns with PR objectives
The addition of com.moclojer.components.logs import supports the transition to the new logging system.
29-33: 🧹 Nitpick (assertive)
Enhance function documentation
The docstring should be more descriptive about parameters and return value.
(defn inspect-if
- "Inspects if condition is met"
+ "Conditionally inspects a value and optional arguments.
+
+ Parameters:
+ c - condition to evaluate
+ v - value to inspect if condition is true
+ a - optional sequence of additional values to inspect
+
+ Returns:
+ The result of inspect(v, a) if c is true, nil otherwise."
[c v & a]
(when c
(apply inspect v a)))Likely invalid or redundant comment.
| (defn inspect | ||
| "Inspects a variable's contents and returns it without modifying its value." | ||
| [v & a] | ||
| (let [v (if (instance? clojure.lang.Atom v) @v v)] | ||
| (if (and (seq? v) (> (count v) 0)) | ||
| (doseq [val v] | ||
| (logs/log :info val)) | ||
| (logs/log :info "Inspect value: " v)) | ||
| (when a | ||
| (doseq [arg a] | ||
| (logs/log :info "Additional arg:" | ||
| (if (instance? clojure.lang.Atom arg) | ||
| @arg | ||
| arg)))) | ||
| v)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use trace level logging for consistency
The function should use logs/trace instead of logs/log :info to align with the PR objectives and maintain consistency with other files.
(defn inspect
"Inspects a variable's contents and returns it without modifying its value."
[v & a]
(let [v (if (instance? clojure.lang.Atom v) @v v)]
(if (and (seq? v) (> (count v) 0))
(doseq [val v]
- (logs/log :info val))
+ (logs/trace ::inspect val))
- (logs/log :info "Inspect value: " v))
+ (logs/trace ::inspect {:value v}))
(when a
(doseq [arg a]
- (logs/log :info "Additional arg:"
- (if (instance? clojure.lang.Atom arg)
- @arg
- arg))))
+ (logs/trace ::inspect-arg
+ {:arg (if (instance? clojure.lang.Atom arg) @arg arg)})))
v))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn inspect | |
| "Inspects a variable's contents and returns it without modifying its value." | |
| [v & a] | |
| (let [v (if (instance? clojure.lang.Atom v) @v v)] | |
| (if (and (seq? v) (> (count v) 0)) | |
| (doseq [val v] | |
| (logs/log :info val)) | |
| (logs/log :info "Inspect value: " v)) | |
| (when a | |
| (doseq [arg a] | |
| (logs/log :info "Additional arg:" | |
| (if (instance? clojure.lang.Atom arg) | |
| @arg | |
| arg)))) | |
| v)) | |
| (defn inspect | |
| "Inspects a variable's contents and returns it without modifying its value." | |
| [v & a] | |
| (let [v (if (instance? clojure.lang.Atom v) @v v)] | |
| (if (and (seq? v) (> (count v) 0)) | |
| (doseq [val v] | |
| (logs/trace ::inspect val)) | |
| (logs/trace ::inspect {:value v})) | |
| (when a | |
| (doseq [arg a] | |
| (logs/trace ::inspect-arg | |
| {:arg (if (instance? clojure.lang.Atom arg) @arg arg)}))) | |
| v)) |
src/dev/utils.clj
Outdated
| (defn get-allowed-ns [] | ||
| (->> (all-ns) | ||
| (map ns-name) | ||
| (map name) | ||
| (filter #(and (str/starts-with? % "com.moclojer") | ||
| (not (str/ends-with? % "logs")))) | ||
| (map symbol) | ||
| (into []))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optimize namespace filtering performance
The current implementation creates multiple intermediate sequences. Consider using transducers for better performance.
(defn- get-allowed-ns []
(->> (all-ns)
- (map ns-name)
- (map name)
- (filter #(and (str/starts-with? % "com.moclojer")
- (not (str/ends-with? % "logs"))))
- (map symbol)
- (into []))
+ (into []
+ (comp
+ (map (comp name ns-name))
+ (filter #(and (str/starts-with? % "com.moclojer")
+ (not (str/ends-with? % "logs"))))
+ (map symbol))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn get-allowed-ns [] | |
| (->> (all-ns) | |
| (map ns-name) | |
| (map name) | |
| (filter #(and (str/starts-with? % "com.moclojer") | |
| (not (str/ends-with? % "logs")))) | |
| (map symbol) | |
| (into []))) | |
| (defn get-allowed-ns [] | |
| (->> (all-ns) | |
| (into [] | |
| (comp | |
| (map (comp name ns-name)) | |
| (filter #(and (str/starts-with? % "com.moclojer") | |
| (not (str/ends-with? % "logs")))) | |
| (map symbol))))) |
src/dev/utils.clj
Outdated
| (start-system-dev! sys-atom sys-map true) | ||
| (trace-all-ns @sys-atom)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error handling for trace-all-ns call
The trace-all-ns call should be wrapped in error handling to prevent system startup failures.
([sys-atom sys-map]
(start-system-dev! sys-atom sys-map true)
- (trace-all-ns @sys-atom))
+ (try
+ (trace-all-ns @sys-atom)
+ (catch Exception e
+ (logs/error ::trace-setup-failed {:error e}))))Committable suggestion skipped: line range outside the PR's diff.
src/dev/utils.clj
Outdated
| (defn trace-all-ns | ||
| ([] | ||
| (trace-all-ns {:config {:env :dev}})) | ||
| ([config] | ||
| (let [env (get-in config [:config :env]) | ||
| a-ns (get-allowed-ns) | ||
| fn-names (atom [])] | ||
| (doseq [curr-ns a-ns] | ||
| (doseq [[sym v] (ns-interns curr-ns)] | ||
| (let [arglists (or (:arglists (meta v)) []) | ||
| arg-names (map keyword (or (first arglists) []))] | ||
| (swap! fn-names conj (str sym)) | ||
| (alter-var-root v | ||
| (fn [orig-fn] | ||
| (if (fn? orig-fn) | ||
| (with-meta | ||
| (fn [& args] | ||
| (let [arg-map (zipmap arg-names args) | ||
| result (apply orig-fn args)] | ||
| (com.moclojer.components.logs/trace sym arg-map result) | ||
| (utils/inspect-if (= env :dev) result))) | ||
| (meta v)) | ||
| orig-fn))))))))) |
There was a problem hiding this comment.
Multiple improvements needed for trace-all-ns function
- The function currently traces private functions via
ns-interns. Consider usingns-publicsinstead to respect function visibility. - The environment check on line 39 appears reversed based on the project's requirements.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))
([config]
(let [env (get-in config [:config :env])
a-ns (get-allowed-ns)
fn-names (atom [])]
(doseq [curr-ns a-ns]
- (doseq [[sym v] (ns-interns curr-ns)]
+ (doseq [[sym v] (ns-publics curr-ns)]
(let [arglists (or (:arglists (meta v)) [])
arg-names (map keyword (or (first arglists) []))]
(swap! fn-names conj (str sym))
(alter-var-root v
(fn [orig-fn]
(if (fn? orig-fn)
(with-meta
(fn [& args]
(let [arg-map (zipmap arg-names args)
result (apply orig-fn args)]
(com.moclojer.components.logs/trace sym arg-map result)
- (utils/inspect-if (= env :dev) result)))
+ (utils/inspect-if (not= env :dev) result)))
(meta v))
orig-fn)))))))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn trace-all-ns | |
| ([] | |
| (trace-all-ns {:config {:env :dev}})) | |
| ([config] | |
| (let [env (get-in config [:config :env]) | |
| a-ns (get-allowed-ns) | |
| fn-names (atom [])] | |
| (doseq [curr-ns a-ns] | |
| (doseq [[sym v] (ns-interns curr-ns)] | |
| (let [arglists (or (:arglists (meta v)) []) | |
| arg-names (map keyword (or (first arglists) []))] | |
| (swap! fn-names conj (str sym)) | |
| (alter-var-root v | |
| (fn [orig-fn] | |
| (if (fn? orig-fn) | |
| (with-meta | |
| (fn [& args] | |
| (let [arg-map (zipmap arg-names args) | |
| result (apply orig-fn args)] | |
| (com.moclojer.components.logs/trace sym arg-map result) | |
| (utils/inspect-if (= env :dev) result))) | |
| (meta v)) | |
| orig-fn))))))))) | |
| (defn trace-all-ns | |
| ([] | |
| (trace-all-ns {:config {:env :prod}})) | |
| ([config] | |
| (let [env (get-in config [:config :env]) | |
| a-ns (get-allowed-ns) | |
| fn-names (atom [])] | |
| (doseq [curr-ns a-ns] | |
| (doseq [[sym v] (ns-publics curr-ns)] | |
| (let [arglists (or (:arglists (meta v)) []) | |
| arg-names (map keyword (or (first arglists) []))] | |
| (swap! fn-names conj (str sym)) | |
| (alter-var-root v | |
| (fn [orig-fn] | |
| (if (fn? orig-fn) | |
| (with-meta | |
| (fn [& args] | |
| (let [arg-map (zipmap arg-names args) | |
| result (apply orig-fn args)] | |
| (com.moclojer.components.logs/trace sym arg-map result) | |
| (utils/inspect-if (not= env :dev) result))) | |
| (meta v)) | |
| orig-fn))))))))) |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/dev/utils.clj(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (3)
src/dev/utils.clj (3)
26-39:
Protect private functions and add error handling
Several improvements needed for the tracing implementation:
- Use
ns-publicsinstead ofns-internsto respect function privacy - Add error handling for tracing operations
(doseq [curr-ns a-ns]
- (doseq [[sym v] (ns-interns curr-ns)]
+ (doseq [[sym v] (ns-publics curr-ns)]
(try
(let [arglists (or (:arglists (meta v)) [])
arg-names (map keyword (or (first arglists) []))]
(swap! fn-names conj (str sym))
(alter-var-root v
(fn [orig-fn]
(if (fn? orig-fn)
(with-meta
(fn [& args]
(com.moclojer.components.logs/trace
sym (zipmap arg-names args) (apply orig-fn args)))
(meta v))
orig-fn))))
+ (catch Exception e
+ (logs/error ::trace-setup-failed
+ {:namespace curr-ns
+ :symbol sym
+ :error e}))))Likely invalid or redundant comment.
44-45:
Add error handling for system tracing
Protect system startup from tracing failures.
(start-system-dev! sys-atom sys-map true)
- (trace-all-ns @sys-atom))
+ (try
+ (trace-all-ns @sys-atom)
+ (catch Exception e
+ (logs/error ::system-trace-failed {:error e})
+ ;; Continue with system startup even if tracing fails
+ @sys-atom)))Likely invalid or redundant comment.
10-17: 🧹 Nitpick (assertive)
Add docstring to explain namespace filtering logic
The get-allowed-ns function would benefit from a docstring explaining its purpose and filtering criteria.
(defn get-allowed-ns []
+ "Returns a vector of namespace symbols that should be traced.
+ Includes namespaces starting with 'com.moclojer' while excluding
+ those ending with 'logs' to prevent recursive tracing.
+ Uses transducers for efficient filtering."
(->> (all-ns)Likely invalid or redundant comment.
src/dev/utils.clj
Outdated
| orig-fn)))))) | ||
| (utils/inspect-if (= env :dev) fn-names)))) |
There was a problem hiding this comment.
Reverse environment check for inspection
The inspection should follow the same environment logic as tracing.
- (utils/inspect-if (= env :dev) fn-names))))
+ (utils/inspect-if (not= env :dev) fn-names))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| orig-fn)))))) | |
| (utils/inspect-if (= env :dev) fn-names)))) | |
| orig-fn)))))) | |
| (utils/inspect-if (not= env :dev) fn-names)))) |
src/dev/utils.clj
Outdated
| (comment | ||
| (trace-all-ns) | ||
|
|
||
| (trace-all-ns {:config {:env :prod}})) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance example documentation
Add more context to the example usage to clarify when each approach should be used.
(comment
+ ;; Default usage (not recommended for production)
(trace-all-ns)
+ ;; Recommended usage for production environments
(trace-all-ns {:config {:env :prod}}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (comment | |
| (trace-all-ns) | |
| (trace-all-ns {:config {:env :prod}})) | |
| (comment | |
| ;; Default usage (not recommended for production) | |
| (trace-all-ns) | |
| ;; Recommended usage for production environments | |
| (trace-all-ns {:config {:env :prod}})) |
src/dev/utils.clj
Outdated
| (defn trace-all-ns | ||
| ([] | ||
| (trace-all-ns {:config {:env :dev}})) | ||
| ([config] |
There was a problem hiding this comment.
Reverse default environment for tracing
Based on the project requirements, tracing should primarily occur in non-dev environments.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (defn trace-all-ns | |
| ([] | |
| (trace-all-ns {:config {:env :dev}})) | |
| ([config] | |
| (defn trace-all-ns | |
| ([] | |
| (trace-all-ns {:config {:env :prod}})) | |
| ([config] |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/dev/utils.clj(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (6)
src/dev/utils.clj (6)
2-8: LGTM: Clean namespace declarations
The circular dependency issue has been resolved and the imports are properly organized.
19-22:
Reverse default environment for tracing
Based on the project requirements, tracing should primarily occur in non-dev environments.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))53-54:
Add error handling for trace-all-ns call
The trace-all-ns call should be wrapped in error handling to prevent system startup failures.
(start-system-dev! sys-atom sys-map true)
- (trace-all-ns @sys-atom))
+ (try
+ (trace-all-ns @sys-atom)
+ (catch Exception e
+ (logs/error ::trace-setup-failed {:error e}))))73-91: 🧹 Nitpick (assertive)
Enhance example documentation
The examples would be more helpful with additional context explaining their purpose and expected usage scenarios.
(comment
+ ;; Basic usage - not recommended for production
(trace-all-ns)
+ ;; Recommended usage for production environments
(trace-all-ns {:config {:env :prod}})
+ ;; Example of manual tracing - useful for understanding the tracing format
(let [args []]10-17: 🧹 Nitpick (assertive)
Add documentation for get-allowed-ns function
The function implementation looks good with efficient use of transducers, but it would benefit from a docstring explaining its purpose and return value.
(defn get-allowed-ns []
+ "Returns a vector of namespace symbols that start with 'com.moclojer' but exclude those ending with 'logs'.
+ Used to determine which namespaces should have tracing enabled."
(->> (all-ns)Likely invalid or redundant comment.
27-28: 🧹 Nitpick (assertive)
Consider using ns-publics instead of ns-interns
Using ns-interns exposes private functions for tracing. Consider using ns-publics to respect function visibility unless there's a specific requirement to trace private functions.
- (doseq [[sym v] (ns-interns curr-ns)]
- (when (and (var? v) (fn? @v))
+ (doseq [[sym v] (ns-publics curr-ns)]
+ (when (fn? @v)Likely invalid or redundant comment.
| (catch Exception e | ||
| (println "Failed to trace" sym (.getMessage e)))))))) | ||
| (utils/inspect-if (= env :dev) @fn-names)))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging
The current error handling only prints to stdout. Consider using the logging system for consistency.
(catch Exception e
- (println "Failed to trace" sym (.getMessage e))))))))
+ (logs/error ::trace-failed
+ {:namespace (str curr-ns)
+ :function (str sym)
+ :error (.getMessage e)})))))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (catch Exception e | |
| (println "Failed to trace" sym (.getMessage e)))))))) | |
| (utils/inspect-if (= env :dev) @fn-names)))) | |
| (catch Exception e | |
| (logs/error ::trace-failed | |
| {:namespace (str curr-ns) | |
| :function (str sym) | |
| :error (.getMessage e)}))))))) | |
| (utils/inspect-if (= env :dev) @fn-names)))) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/dev/utils.clj(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (7)
src/dev/utils.clj (7)
2-8: LGTM! Circular dependency resolved
The namespace declarations have been cleaned up, removing the circular dependency previously identified. The current imports are appropriate for the functionality.
54-55: 🛠️ Refactor suggestion
Add error handling for tracing setup
Tracing failures shouldn't impact system startup. Wrap the trace-all-ns call in error handling.
(start-system-dev! sys-atom sys-map true)
- (trace-all-ns @sys-atom))
+ (try
+ (trace-all-ns @sys-atom)
+ (catch Exception e
+ (logs/error ::trace-setup-failed {:error e}))))Likely invalid or redundant comment.
50-50:
Reverse inspection condition
The inspection condition should match the tracing environment logic.
- (utils/inspect-if (= env :dev) @fn-names))))
+ (utils/inspect-if (not= env :dev) @fn-names))))Likely invalid or redundant comment.
74-92: 🧹 Nitpick (assertive)
Enhance example documentation
The examples would benefit from better documentation explaining their purpose and expected output.
(comment
+ ;; Basic usage - enables tracing with default configuration (not recommended for production)
(trace-all-ns)
+ ;; Recommended usage - explicitly enable tracing for production environment
(trace-all-ns {:config {:env :prod}})
+ ;; Example of manual tracing (for testing/development purposes only)
(let [args []]⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
46-49: 🛠️ Refactor suggestion
Improve error handling with proper logging
Replace println with proper logging for error cases.
(try
(alter-var-root v traced-fn)
(catch Exception e
- (println "Failed to trace" sym (.getMessage e))))))))
+ (logs/error ::trace-failed
+ {:namespace (str curr-ns)
+ :function (str sym)
+ :error (.getMessage e)})))))))Likely invalid or redundant comment.
10-17: 🧹 Nitpick (assertive)
Add docstring to explain namespace filtering logic
While the implementation is efficient using transducers, a docstring would help explain the function's purpose and filtering criteria.
(defn- get-allowed-ns []
+ "Returns a vector of namespace symbols that start with 'com.moclojer' but exclude those ending with 'logs'.
+ Used to determine which namespaces should have tracing enabled."
(->> (all-ns)Likely invalid or redundant comment.
19-22:
Reverse default environment for tracing
Based on the project requirements, tracing should primarily occur in non-dev environments.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
src/dev/utils.clj
Outdated
| (doseq [[sym v] (ns-interns curr-ns)] | ||
| (when (and (var? v) (fn? @v)) | ||
| (let [arglists (-> v meta :arglists first) | ||
| arg-names (map keyword (or arglists [])) | ||
| solved-fn (str curr-ns "/" sym) | ||
| traced-fn (fn [f] | ||
| (with-meta | ||
| (fn [& args] | ||
| (com.moclojer.components.logs/trace | ||
| ::traced-fn | ||
| {:ns (str curr-ns) | ||
| :fn (str sym) | ||
| :args (zipmap arg-names args)} | ||
| ::traced-fn | ||
| (if (> (count args) 0) | ||
| (f args) | ||
| (f)))) | ||
| (meta f)))] |
There was a problem hiding this comment.
Fix argument handling in traced function
The current implementation has several issues:
- Using
ns-internsexposes private functions - consider usingns-publicsinstead - Argument handling in the traced function is incorrect
- (doseq [[sym v] (ns-interns curr-ns)]
+ (doseq [[sym v] (ns-publics curr-ns)]
(when (and (var? v) (fn? @v))
(let [arglists (-> v meta :arglists first)
arg-names (map keyword (or arglists []))
solved-fn (str curr-ns "/" sym)
traced-fn (fn [f]
(with-meta
(fn [& args]
(com.moclojer.components.logs/trace
::traced-fn
{:ns (str curr-ns)
:fn (str sym)
:args (zipmap arg-names args)}
::traced-fn
- (if (> (count args) 0)
- (f args)
- (f))))
+ (apply f args)))
(meta f)))]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (doseq [[sym v] (ns-interns curr-ns)] | |
| (when (and (var? v) (fn? @v)) | |
| (let [arglists (-> v meta :arglists first) | |
| arg-names (map keyword (or arglists [])) | |
| solved-fn (str curr-ns "/" sym) | |
| traced-fn (fn [f] | |
| (with-meta | |
| (fn [& args] | |
| (com.moclojer.components.logs/trace | |
| ::traced-fn | |
| {:ns (str curr-ns) | |
| :fn (str sym) | |
| :args (zipmap arg-names args)} | |
| ::traced-fn | |
| (if (> (count args) 0) | |
| (f args) | |
| (f)))) | |
| (meta f)))] | |
| (doseq [[sym v] (ns-publics curr-ns)] | |
| (when (and (var? v) (fn? @v)) | |
| (let [arglists (-> v meta :arglists first) | |
| arg-names (map keyword (or arglists [])) | |
| solved-fn (str curr-ns "/" sym) | |
| traced-fn (fn [f] | |
| (with-meta | |
| (fn [& args] | |
| (com.moclojer.components.logs/trace | |
| ::traced-fn | |
| {:ns (str curr-ns) | |
| :fn (str sym) | |
| :args (zipmap arg-names args)} | |
| ::traced-fn | |
| (apply f args))) | |
| (meta f)))] |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/dev/api/dev.clj(4 hunks)src/dev/utils.clj(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (8)
src/dev/api/dev.clj (1)
18-18: LGTM: Logger component integration
The logger component is correctly integrated into the system map with proper dependency on the config component.
src/dev/utils.clj (7)
10-17: LGTM: Namespace filtering implementation
The get-allowed-ns function correctly filters namespaces and uses transducers for better performance.
54-55: Verify logger initialization before tracing
Ensure the logger component is properly initialized before attempting to trace.
✅ Verification successful
Based on the search results, I can see that the logger initialization is properly handled in the system startup process. The key findings are:
- The logger component is properly initialized as part of the system components:
:logger (component/using (components/new-logger) [:config])- The logger is started before tracing in the system startup sequence:
(defn start-system! [system-map]
(->> system-map
component/start
(reset! system-atom))
(trace-all-ns))- The logger is a dependency for other components and is initialized early in the component lifecycle.
Logger initialization is properly handled before tracing
The logger component is properly initialized as part of the system startup sequence before any tracing occurs. The component system ensures proper dependency ordering, and the logger is started before trace-all-ns is called.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if logger initialization is handled properly
# Look for error handling around logger initialization
rg -A 5 "new-logger|start-system"
Length of output: 30336
43-43:
Fix environment check for inspection
The inspection should follow the same environment logic as tracing.
- (utils/inspect-if (= env :dev) @fn-names))))
+ (utils/inspect-if (not= env :dev) @fn-names))))Likely invalid or redundant comment.
39-42: 🛠️ Refactor suggestion
Enhance error handling with proper logging
Replace println with proper logging for consistency and better error tracking.
(catch Exception e
- (println "Failed to trace" sym (.getMessage e))))))))
+ (logs/error ::trace-failed
+ {:namespace (str curr-ns)
+ :function (str sym)
+ :error (.getMessage e)})))))))Likely invalid or redundant comment.
19-22:
Fix default environment for tracing
Based on the project requirements, tracing should primarily occur in non-dev environments.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
27-37:
Multiple improvements needed for function tracing
- Using
ns-internsexposes private functions - Argument handling in the traced function is incorrect
- (doseq [[sym v] (ns-interns curr-ns)]
+ (doseq [[sym v] (ns-publics curr-ns)]
(when (and (var? v) (fn? @v))
(let [arglists (-> v meta :arglists first)
arg-names (map keyword (or arglists []))
solved-fn (str curr-ns "/" sym)
traced-fn (fn [f]
- (fn [n]
+ (fn [& args]
(com.moclojer.components.logs/trace
::traced-fn
- (zipmap arg-names n)
- (f n))))]
+ (zipmap arg-names args)
+ (apply f args))))]Likely invalid or redundant comment.
67-81: 🧹 Nitpick (assertive)
Enhance example documentation
Add more context to the example usage to clarify when each approach should be used.
(comment
+ ;; Default usage (for production environments)
(trace-all-ns)
+ ;; Explicit production configuration
(trace-all-ns {:config {:env :prod}})
+ ;; Example of manual tracing (for debugging)
(let [args []]
(com.moclojer.components.logs/trace
::traced-fn
{:test? :test}
(trace-all-ns args)))⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
src/dev/api/dev.clj
Outdated
| (component/start | ||
| (:logger @sys-atom)) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding comprehensive logger usage examples
While the basic logger initialization is shown, it would be helpful to include examples demonstrating the new Telemere logging capabilities.
;; Example logger usage with Telemere
(let [logger (:logger @sys-atom)]
;; Log with different levels
(com.moclojer.components.logs/trace ::example {:data "test"} (+ 1 2))
(com.moclojer.components.logs/debug ::example {:message "debugging"})
(com.moclojer.components.logs/info ::example {:status "running"}))
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
src/back/api/server.clj(1 hunks)src/dev/api/dev.clj(4 hunks)src/dev/utils.clj(2 hunks)src/job_ops/server.clj(1 hunks)test/back/back/integration/components/utils.clj(0 hunks)
💤 Files with no reviewable changes (1)
- test/back/back/integration/components/utils.clj
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (12)
src/dev/api/dev.clj (1)
45-47: 🧹 Nitpick (assertive)
Consider adding comprehensive logger usage examples
While accessing the logger component is shown, it would be helpful to include example usage of common logging functions for development purposes.
src/back/api/server.clj (3)
17-17: LGTM: Logger component integration
The logger component is properly integrated with the correct dependency on the config component.
9-9:
Remove development utilities from production code
Importing dev.utils in production code can lead to unintended side effects and potential security risks.
42-43:
Remove tracing from production startup
Using trace-all-ns in production could impact performance and potentially expose sensitive information.
src/dev/utils.clj (8)
2-8: LGTM! Dependencies look appropriate
The namespace declarations are clean and include all necessary dependencies for the tracing functionality.
10-17: Add docstring to explain namespace filtering logic
The function would benefit from a docstring explaining its purpose and filtering criteria.
(defn get-allowed-ns []
+ "Returns a vector of namespace symbols that should be traced.
+ Includes namespaces starting with 'com.moclojer' but excludes those ending with 'logs'
+ to prevent recursive tracing."
(->> (all-ns)39-41: Improve error handling using logging system
Replace println with proper logging for consistency and better error tracking.
(catch Exception e
- (println "Failed to trace" sym (.getMessage e))))))))
+ (logs/error ::trace-failed
+ {:namespace (str curr-ns)
+ :function (str sym)
+ :error (.getMessage e)})))))))52-53: Add error handling for trace-all-ns call
Protect system startup from tracing failures.
(reset! sys-atom))
- (trace-all-ns @sys-atom)))
+ (try
+ (trace-all-ns @sys-atom)
+ (catch Exception e
+ (logs/error ::trace-setup-failed {:error e})))))65-79: Enhance example documentation
Add more context to clarify when each approach should be used.
(comment
+ ;; Default usage (not recommended for production)
(trace-all-ns)
+ ;; Recommended usage for production environments
(trace-all-ns {:config {:env :prod}})
+ ;; Example of manual tracing (for testing purposes)
(let [args []]
(logs/trace41-41:
Reverse environment check for inspection
The inspection should follow the same environment logic as tracing.
- (utils/inspect-if (= env :dev) @fn-names))))
+ (utils/inspect-if (not= env :dev) @fn-names))))Likely invalid or redundant comment.
19-22:
Fix default environment for tracing
Based on the project requirements, tracing should primarily occur in non-dev environments.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
26-28:
Use ns-publics instead of ns-interns
Using ns-interns exposes private functions for tracing, which violates encapsulation. Switch to ns-publics to respect function visibility.
(doseq [curr-ns a-ns]
- (doseq [[sym v] (ns-interns curr-ns)]
+ (doseq [[sym v] (ns-publics curr-ns)]Likely invalid or redundant comment.
| @@ -16,7 +16,7 @@ | |||
| [:config :sentry]))) | |||
|
|
|||
| (defn start-system! [system-map] | |||
| (components/setup-logger [["*" :info]] :auto :prod) | |||
| #_(components/setup-logger [["*" :info]] :auto :prod) | |||
There was a problem hiding this comment.
Critical: Logger setup is commented out without replacement
The logger setup is commented out but no alternative logging configuration is provided. This could lead to missing logs in production.
Consider adding the new logger component similar to other server files:
(defn build-system-map []
(component/system-map
:config (components/new-config "back/config.edn")
+ :logger (component/using (components/new-logger) [:config])
:sentry (component/using (components/new-sentry) [:config])
:mq (component/using
(components/new-mq workers false)
[:config :sentry])))Committable suggestion skipped: line range outside the PR's diff.
| @@ -13,7 +14,9 @@ | |||
|
|
|||
| (defn build-system-map [] | |||
| (component/system-map | |||
| :env :prod | |||
There was a problem hiding this comment.
Reconsider using :prod environment in development namespace
Setting :env :prod in a development-only namespace could lead to confusion and potential issues. Consider using :dev for development environments.
- :env :prod
+ :env :dev📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| :env :prod | |
| :env :dev |
| [{:channel "domains.verification.fired" | ||
| :event {} | ||
| ;; every 2 minutes | ||
| :sleep 120000} | ||
| {:channel "yml.unified.verification.fired" | ||
| :event {} | ||
| ;; every 5 minutes | ||
| :sleep 300000}] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document MQ channel configuration
Consider adding documentation for the MQ channels explaining their purpose and the rationale behind the sleep intervals.
Add comments explaining:
- Purpose of each verification channel
- Impact of sleep intervals on system behavior
- Any dependencies or prerequisites
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/back/api/logic/mocks.clj(1 hunks)src/dev/utils.clj(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/dev/utils.clj (1)
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
🔇 Additional comments (8)
src/back/api/logic/mocks.clj (1)
Line range hint 1-147: Verify logging implementation in dependent controllers.
Since this file contains core business logic, ensure that the controllers using these functions (particularly update, enable/disable, and status update functions) have been updated to use the new Telemere logging as per PR objectives.
Let's verify the logging implementation in the controllers:
✅ Verification successful
Let me check the specific logging implementation in the mocks controller to verify if it properly logs these critical operations.
Let me check the complete mocks controller file to see the logging context around these operations.
Proper Telemere logging is implemented in the controllers
The controllers are correctly using the new Telemere logging system through com.moclojer.components.logs for all critical mock operations:
- All functions using
logic.mocksoperations have correspondinglogs/tracecalls - Each log entry includes:
- Proper namespace-qualified event names (e.g.
::update-mock!) - Relevant context data (mock IDs, user IDs, status changes)
- Logging before the actual operations
- Proper namespace-qualified event names (e.g.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if controllers using these mock functions have proper logging.
# Search for usage of mock logic functions in controllers
echo "Checking for mock function usage and logging patterns in controllers:"
rg -A 3 "logic\.mocks/(update|enable|disable|publish|unpublish|update-dns-status|update-unification-status)" "src/back/api/controllers/"
# Search for new logging pattern
echo -e "\nVerifying Telemere logging implementation:"
rg "logs/(trace|debug|info|warn|error)" "src/back/api/controllers/"
Length of output: 3160
Script:
#!/bin/bash
# Check the specific logging implementation around mock operations
echo "Checking logging implementation around mock operations:"
rg -B 2 -A 4 "logic\.mocks/(update|enable|disable|publish|unpublish|update-dns-status|update-unification-status)" "src/back/api/controllers/mocks.clj" --context-separator="---"
Length of output: 1667
Script:
#!/bin/bash
# Get the complete mocks controller implementation
echo "Checking complete mocks controller implementation:"
cat "src/back/api/controllers/mocks.clj"
Length of output: 6079
src/dev/utils.clj (7)
2-8: LGTM: Namespace declarations are appropriate
The namespace declarations properly include all required dependencies for the tracing functionality.
46-46:
Reverse environment check for inspection
The inspection should follow the same logic as tracing (active in non-dev environments).
- (utils/inspect-if (= env :dev) @fn-names))))
+ (utils/inspect-if (not= env :dev) @fn-names))))Likely invalid or redundant comment.
19-22:
Fix default environment for tracing
Based on the project requirements, tracing should primarily occur in non-dev environments.
(defn trace-all-ns
([]
- (trace-all-ns {:config {:env :dev}}))
+ (trace-all-ns {:config {:env :prod}}))⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
57-58: 🛠️ Refactor suggestion
Add error handling for trace-all-ns call
Protect system startup from tracing failures.
(reset! sys-atom))
- (trace-all-ns @sys-atom)))
+ (try
+ (trace-all-ns @sys-atom)
+ (catch Exception e
+ (logs/error ::trace-setup-failed {:error e})
+ @sys-atom))))Likely invalid or redundant comment.
70-85: 🧹 Nitpick (assertive)
Enhance example documentation
Add more context to clarify the intended usage.
(comment
+ ;; Not recommended - uses default dev environment
(trace-all-ns)
+ ;; Recommended - explicitly enables tracing for production
(trace-all-ns {:config {:env :prod}})
+ ;; Example of a traced function's behavior
(defn f [& args]
(logs/trace
(keyword "traced-fn" (str 'dev.utils "/" 'prn))
{:test? :test}
(prn args)))
(f)
+ ;; Example of direct trace usage
(logs/trace
::testing-stuff
{:testing? :definitely}
(get-allowed-ns)))⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
10-17: 🧹 Nitpick (assertive)
Add docstring to explain namespace filtering logic
The implementation using transducers is efficient, but adding documentation would improve maintainability.
(defn- get-allowed-ns []
+ "Returns a vector of namespace symbols that should be traced.
+ Includes namespaces starting with 'com.moclojer' while excluding
+ those ending with 'logs' to prevent recursive tracing.
+ Uses transducers for efficient filtering."
(->> (all-ns)Likely invalid or redundant comment.
44-45: 🛠️ Refactor suggestion
Enhance error handling with proper logging
Replace println with structured logging for better error tracking.
(catch Exception e
- (println "Failed to trace" sym (.getMessage e))))))))
+ (logs/error ::trace-failed
+ {:namespace (str curr-ns)
+ :function (str sym)
+ :error (.getMessage e)})))))))Likely invalid or redundant comment.
| (ns back.api.logic.mocks | ||
| (:refer-clojure :exclude [update]) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding docstring to the update function.
The exclusion of clojure.core/update is correct as it prevents naming conflicts with the local update function. This follows Clojure best practices for namespace management.
Consider adding a docstring to the update function to clarify its specific purpose in managing mock content:
(defn update
+ "Updates the content of a mock and returns a map containing only the ID and updated content."
[mock {:keys [content]}]
(-> (assoc mock :mock/content content)
(select-keys [:mock/id :mock/content])))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
📒 Files selected for processing (1)
test/back/back/integration/api/wrap_traces_test.clj(1 hunks)
🔇 Additional comments (4)
test/back/back/integration/api/wrap_traces_test.clj (4)
1-11: LGTM! Well-organized namespace declaration.
The imports are logically grouped and all dependencies are properly utilized in the code.
13-23: Add docstrings to helper functions
Both helper functions would benefit from clear documentation explaining their purpose and return values.
Also applies to: 24-30
41-42: Verify tracing implementation
Let's verify the actual implementation of the tracing functionality to ensure the test is valid.
✅ Verification successful
Based on the shell script results, I can now provide a final response since I have sufficient information about the tracing implementation:
Tracing implementation is properly integrated
The test is valid as it correctly verifies the tracing functionality:
- The
trace-all-nsfunction insrc/dev/utils.cljimplements comprehensive tracing by:- Gathering allowed namespaces
- Wrapping functions with
logs/tracecontext - Tracking function names, arguments, and locations
- The tracing is used throughout the codebase in controllers and DB operations
- The test correctly compares local functions with traced functions to ensure proper wrapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of trace-all-ns and related functions
# Look for the tracing implementation
echo "Searching for trace-all-ns implementation..."
rg -A 10 "trace-all-ns"
# Look for actual trace usage
echo "Searching for trace usage in the codebase..."
rg -A 5 "trace"
# Look for logging configuration
echo "Searching for logging configuration..."
fd -e edn -e clj -x rg -l "logger|trace|log"
Length of output: 29286
24-30: 🛠️ Refactor suggestion
Simplify save-local-fns implementation using sequence operations
The current implementation using atom and nested doseq can be simplified using Clojure's sequence operations.
Consider this more idiomatic implementation:
-(defn- save-local-fns []
- (let [a-ns (u/get-allowed-ns)
- m (atom [])]
- (doseq [d-ns a-ns]
- (doseq [[func _] (ns-publics d-ns)]
- (swap! m conj (str d-ns "/" func))))
- @m))
+(defn- save-local-fns []
+ (->> (u/get-allowed-ns)
+ (mapcat (fn [ns]
+ (map (fn [[func _]]
+ (str ns "/" func))
+ (ns-publics ns))))
+ (into [])))Likely invalid or redundant comment.
| (defflow | ||
| fwrap-all-ns-functions | ||
| {:init (utils/start-system! create-and-start-components) | ||
| :cleanup utils/stop-system! | ||
| :fail-fast? true} | ||
|
|
||
| (flow | ||
| "will try to wrap all desired ns functions into a trace container | ||
| and will compare them just by name" | ||
| (let [local-fns (save-local-fns) | ||
| expected-fns (u/trace-all-ns)] | ||
| (match? (matchers/equals local-fns) expected-fns)))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance test coverage with error cases and logging verification
The test only verifies function names but doesn't test:
- Error cases (e.g., invalid namespaces)
- Whether the wrapped functions actually log traces
Consider adding additional test flows to verify these scenarios.
Example structure:
(defflow test-invalid-namespace
;; Test wrapping functions from non-existent namespace
)
(defflow test-trace-logging
;; Test that wrapped functions produce expected log output
)| (flow | ||
| "will try to wrap all desired ns functions into a trace container | ||
| and will compare them just by name" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve multiline string formatting
The test description string should use proper string formatting for better readability.
- "will try to wrap all desired ns functions into a trace container
- and will compare them just by name"
+ "will try to wrap all desired ns functions into a trace container and will compare them just by name"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (flow | |
| "will try to wrap all desired ns functions into a trace container | |
| and will compare them just by name" | |
| (flow | |
| "will try to wrap all desired ns functions into a trace container and will compare them just by name" |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/back/back/integration/api/create_conflicting_mock_test.clj (1)
The commented-out test is the only validation for conflicting mocks scenario
Based on the analysis:
- The
create_conflicting_mock_test.cljcontains the only test validating theinvalid-wildcarderror case- The implementation in
src/back/api/controllers/mocks.cljshows this is a real error condition that needs testing- Other mock-related tests (
create_mock_test.clj) only cover the happy path scenarios- No other tests in the codebase validate wildcard conflicts
The test should be fixed and enabled to ensure proper coverage of this error handling scenario.
🔗 Analysis chain
Line range hint
1-68: Verify impact on test coverageThe test appears to be the main validation for handling conflicting mocks. Disabling it could leave critical functionality untested.
Let's analyze the test coverage impact:
Let me gather more information about the test coverage and related tests.
Let me check the other mock-related test files to see if they cover similar scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find the related controller/handler code rg -A 5 "invalid-wildcard" src/ # Look for similar error handling patterns ast-grep --pattern 'cause "invalid-wildcard"'Length of output: 655
Script:
#!/bin/bash # Find all test files related to mocks fd "mock.*test\.clj$" # Look for other tests that might be covering the invalid-wildcard scenario rg -B2 -A2 "invalid-wildcard" test/ # Check if there are any other active tests in the same file rg "defflow" test/back/back/integration/api/create_conflicting_mock_test.cljLength of output: 1187
Script:
#!/bin/bash # Check content of create_mock_test.clj for similar test cases cat test/back/back/integration/api/create_mock_test.clj # Look for any other tests validating wildcard conflicts rg -l "wildcard.*conflict|conflict.*wildcard" test/Length of output: 3781
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
📒 Files selected for processing (1)
test/back/back/integration/api/create_conflicting_mock_test.clj(1 hunks)
| (defflow | ||
| ;; Not working now | ||
| ;; TODO fix the test | ||
| #_(defflow |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using pending test annotation instead of commenting out
Rather than completely disabling the test with #_, consider using your testing framework's pending/skip functionality to maintain visibility of the disabled test in test reports.
Example:
-#_(defflow
+;; TODO: Fix mock conflict handling (Issue #XXX)
+(defflow ^:pendingThis approach:
- Keeps the test visible in reports
- Explicitly shows it as pending/skipped
- Maintains better tracking of disabled tests
Committable suggestion skipped: line range outside the PR's diff.
| ;; Not working now | ||
| ;; TODO fix the test | ||
| #_(defflow |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Commented test reduces critical test coverage for conflict handling
The commented test is the only one specifically testing the conflict prevention mechanism for mock creation. While create_mock_test.clj covers the basic mock creation flow, it doesn't verify the system's ability to prevent duplicate wildcards. This test is crucial as it validates:
- The conflict detection in
create-mock!controller function - The 412 status code response with
invalid-wildcarderror - The proper handling of concurrent mock creation attempts
Action items:
- The test should be fixed rather than commented out as it covers unique functionality
- If there are specific issues with the test, they should be documented in a ticket
- Consider adding logging statements in the controller to help debug the test failure
🔗 Analysis chain
Provide more context about the test failure
The test has been commented out with a vague TODO comment. This reduces test coverage for the mock creation functionality, particularly for the conflict handling scenarios. Please:
- Document the specific issue causing the test failure
- Add a JIRA/GitHub issue reference if one exists
- Provide an estimated timeline for fixing the test
Let's check if this is the only test covering mock conflicts:
Let me gather more information about the test coverage for mock-related functionality.
Let me check the create_mock_test.clj to understand if there's any overlap in test coverage.
Let me check the mock controller to understand the validation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests related to mock conflicts
rg -l "conflicting.*mock" --type clojure test/
Length of output: 112
Script:
#!/bin/bash
# Search for all mock-related tests to understand coverage
rg -l "mock" --type clojure test/
# Also check for specific mock creation tests
rg -l "create.*mock" --type clojure test/
# Look at the actual test content to understand what's being tested
cat test/back/back/integration/api/create_conflicting_mock_test.clj
Length of output: 4777
Script:
#!/bin/bash
# Check the content of the main mock creation test
cat test/back/back/integration/api/create_mock_test.clj
# Also check if there are any related issues in the codebase
rg -l "invalid-wildcard" --type clojure
Length of output: 3866
Script:
#!/bin/bash
# Check the content of the mocks controller
cat src/back/api/controllers/mocks.clj
# Also look for any recent changes to this file
git log -p --since="1 month ago" src/back/api/controllers/mocks.clj
Length of output: 5968
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
📒 Files selected for processing (1)
src/back/api/server.clj(1 hunks)
🔇 Additional comments (5)
src/back/api/server.clj (5)
13-22: Thorough implementation of filtered namespace gathering.
This helper function accurately captures all namespaces starting with "com.moclojer", excluding those ending in "logs". This approach helps ensure that you don’t unintentionally include unwanted or sensitive namespaces in the tracing process.
68-75: Add or refine documentation for MQ channel configuration.
This MQ configuration block appears to define periodic tasks (every 2 minutes, every 5 minutes) for domain/yml verifications. It would be helpful to explain the business logic behind these queue intervals, how they are used, and any dependencies.
60-60: Logger component addition looks good.
Registering the new logger component under ":logger" is well-aligned with the established component system. This modular approach cleanly isolates logging configuration.
85-86: Remove or conditionally enable 'trace-all-ns' in production code.
As previously mentioned, invoking 'trace-all-ns' in a live (production) environment can lead to performance issues, verbose logs, and possible leaks of sensitive data. Use environment checks to toggle this tracing only in development.
24-54:
Potential performance overhead & security risk with 'trace-all-ns'.
Altering var roots to wrap every public function in multiple namespaces can be expensive and may log sensitive data. If certain functions process credentials, tokens, or PII, the metainfo might inadvertently get logged. Consider restricting tracing to a development or testing environment only, or whitelisting safer namespaces to reduce risk.
⛔ Skipped due to learnings
Learnt from: Felipe-gsilva
PR: moclojer/moclojer-app#364
File: src/dev/utils.clj:61-87
Timestamp: 2024-12-05T12:32:59.217Z
Learning: In the `moclojer-app` project, the `trace-all-ns` function in `src/dev/utils.clj` is intended to enable tracing primarily in non-development environments (i.e., when `:env` is not `:dev`).
| (catch Exception e | ||
| (println "Failed to trace" sym (.getMessage e)))))))) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refine error handling for trace exceptions.
Currently, the catch block just prints an error message. For production readiness, consider using the logger to record these exceptions so they appear in centralized logs and can be observed more easily.
Felipe-gsilva
requested review from
matheusfrancisco and
avelino
and removed request for
matheusfrancisco
closes #361
Summary by CodeRabbit
New Features
Bug Fixes
com.moclojer/componentsfrom0.1.0to0.1.4.Chores