Unicron add api logs in dynamodb #4894
Conversation
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
lukaszgryglicki
requested review from
ahmedomosanya,
jarias-lfx and
mlehotskylf
WalkthroughAdds API request logging across services: new APILog models and repository in Go, DB-backed middleware wiring, Python ORM/model and middleware hooks to emit three time-bucketed DynamoDB entries per request, plus infra and utility scripts to provision, back up, and query the DynamoDB table. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant GoMiddleware as Go Middleware
participant GoRepo as Go Repository
participant PyMiddleware as Python Middleware
participant PyModel as Python APILog Model
participant DynamoDB
Client->>GoMiddleware: HTTP request (path)
alt Go service with DB logger
GoMiddleware->>GoRepo: LogAPIRequest(ctx, url) [async]
GoRepo->>GoRepo: build 3 APILog items (ALL, daily, monthly)
GoRepo->>DynamoDB: PutItem ×3
DynamoDB-->>GoRepo: OK / Error
end
Client->>PyMiddleware: HTTP request (path)
alt Python service with APILog
PyMiddleware->>PyModel: log_api_request(path)
PyModel->>PyModel: build & save 3 entries (ALL, daily, monthly)
PyModel->>DynamoDB: PutItem ×3
DynamoDB-->>PyModel: OK / Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @cla-backend-go/cmd/server.go:
- Around line 171-176: Increase the timeout and make the DynamoDB log call
asynchronous: replace the 300ms context.WithTimeout with a 1-2 second timeout
(e.g., 1s or 2s) when creating ctx/cancel for the apiLogsRepo.LogAPIRequest
call, then invoke LogAPIRequest inside a new goroutine so it doesn't block the
handler (create the ctx for that goroutine and call defer cancel inside it).
Ensure the goroutine body is wrapped with a full panic recovery (recover) that
covers the entire goroutine function, and reference the existing
apiLogsRepo.LogAPIRequest and r.URL.Path when moving the call to the goroutine
so the log still records the path.
In @utils/aws_edit_table_access.sh:
- Around line 14-26: The script currently writes IAM policy JSON to predictable
/tmp paths (e.g., /tmp/cla-backend-${STAGE}-${AWS_REGION}-lambda.json), which is
insecure; change it to create secure temporary files with mktemp and restrictive
permissions and store their paths in variables BACKEND_POLICY and
BACKEND_GO_POLICY (e.g., BACKEND_POLICY=$(mktemp) && chmod 600
"$BACKEND_POLICY"), then direct the aws --query PolicyDocument output into those
variables and update all subsequent references to use ${BACKEND_POLICY} and
${BACKEND_GO_POLICY} instead of hard-coded /tmp filenames so the files are
non-predictable and readable only by the current user.
- Around line 28-40: The script currently runs iam put-role-policy immediately
for cla-backend-${STAGE}-${AWS_REGION}-lambda and cla-backend-go-${STAGE}-lambda
using /tmp/cla-backend-...-lambda.json files; add pre-flight validation and
confirmation: validate each temp JSON with a JSON parser (e.g., jq -e) and exit
on parse errors, create backups of the existing policies using aws iam
get-role-policy --role-name <role> --policy-name <policy> and save to /tmp
before applying changes, display a summary of the target role/policy and the
file path and prompt the operator to confirm (Y/N) before invoking iam
put-role-policy, and optionally offer a dry-run by running aws iam
simulate-custom-policy or printing the parsed JSON for review; ensure the
changes reference the exact role names
(cla-backend-${STAGE}-${AWS_REGION}-lambdaRole, cla-backend-${STAGE}-lambda,
cla-backend-go-${STAGE}-us-east-2-lambdaRole, cla-backend-go-${STAGE}-lambda)
and abort without making changes if validation or confirmation fails.
In @utils/aws_example_api_logs_usage.sh:
- Around line 1-63: The script mixes executable aws CLI commands (e.g., the aws
--profile lfproduct-dev dynamodb query/put-item commands) with raw JSON example
outputs, which will break shell execution; separate them by either commenting
out the example JSON blocks, wrapping the example outputs in a heredoc (e.g.,
cat <<'EOF' ... EOF) so they are treated as plain text, or move the examples to
a Markdown documentation file; update the sections currently containing the JSON
examples (the query response blocks following the aws query commands)
accordingly so the script only contains runnable commands or properly
quoted/documented output.
🧹 Nitpick comments (3)
utils/aws_edit_table_access.sh (1)
1-40: Add error handling to prevent partial policy updates.The script lacks error checking for AWS CLI commands, which could result in partial updates if one command fails while another succeeds.
⚡ Proposed error handling
#!/bin/bash +set -euo pipefail if [ -z "$AWS_REGION" ]; then echo "AWS_REGION is not set. Please set it before running the script." exit 1 fi if [ -z "$STAGE" ]; then echo "STAGE is not set. Please set it before running the script." exit 1 fi +# Check AWS CLI is available +if ! command -v aws &> /dev/null; then + echo "Error: aws CLI not found" + exit 1 +fi + # ... rest of script with error checking built in via set -eThe
-eflag will cause the script to exit immediately if any AWS command fails, preventing partial updates.utils/aws_create_api_logs_table.sh (1)
65-65: Add quotes around variable expansions for robustness.While unlikely in this context, profile names containing spaces or special characters could cause issues.
Recommended fix
-aws --profile ${PROFILE} dynamodb describe-table --table-name cla-${STAGE}-api-log --region ${AWS_REGION} +aws --profile "${PROFILE}" dynamodb describe-table --table-name "cla-${STAGE}-api-log" --region "${AWS_REGION}"cla-backend-go/api_logs/repository.go (1)
46-48: Consider: Nil context handling masks caller bugs.While the "200% fail-safe" approach prevents panics, replacing
nilcontext withBackground()masks caller bugs. In Go, passingnilcontext typically indicates a programming error that should be caught early.Consider returning an error immediately if
ctxisnil:Alternative nil context handling
func (r *repository) LogAPIRequest(ctx context.Context, url string) error { - // 200% fail-safe: never panic on nil ctx/client if ctx == nil { - ctx = context.Background() + return fmt.Errorf("context cannot be nil") } if r == nil || r.dynamoDBClient == nil { return fmt.Errorf("dynamodb client is nil") }However, if the defensive approach is intentional for this logging use case (where failures should be extremely rare), the current implementation is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
cla-backend-go/api_logs/models.gocla-backend-go/api_logs/repository.gocla-backend-go/cmd/server.gocla-backend/cla/models/dynamo_models.pycla-backend/cla/models/model_interfaces.pycla-backend/cla/routes.pyinfra/backup-dynamodb-tables.shutils/aws_create_api_logs_table.shutils/aws_edit_table_access.shutils/aws_example_api_logs_usage.shutils/example_scan_api_log.sh
🧰 Additional context used
🧬 Code graph analysis (6)
cla-backend-go/api_logs/models.go (2)
cla-backend/cla/models/dynamo_models.py (1)
APILog(5429-5516)cla-backend/cla/models/model_interfaces.py (1)
APILog(2388-2462)
utils/aws_create_api_logs_table.sh (1)
tests/py2go/dynamo.go (1)
STAGE(16-16)
cla-backend/cla/routes.py (2)
cla-backend/cla/models/dynamo_models.py (2)
APILog(5429-5516)log_api_request(5480-5516)cla-backend/cla/models/model_interfaces.py (2)
APILog(2388-2462)log_api_request(2454-2462)
cla-backend-go/cmd/server.go (1)
cla-backend-go/api_logs/repository.go (2)
Repository(23-25)NewRepository(34-39)
cla-backend/cla/models/dynamo_models.py (2)
cla-backend/cla/models/model_interfaces.py (65)
APILog(2388-2462)to_dict(14-21)to_dict(302-309)to_dict(609-616)to_dict(787-794)to_dict(1206-1213)to_dict(1467-1474)to_dict(1722-1729)to_dict(1863-1870)to_dict(1909-1916)to_dict(2011-2018)to_dict(2057-2064)to_dict(2103-2110)to_dict(2155-2156)to_dict(2173-2180)to_dict(2300-2307)to_dict(2347-2354)save(23-29)save(311-315)save(618-622)save(796-800)save(1215-1219)save(1872-1876)save(1918-1922)save(2020-2024)save(2066-2070)save(2112-2116)save(2158-2159)save(2182-2186)save(2309-2313)save(2356-2360)save(2402-2406)load(31-39)load(317-325)load(624-632)load(802-810)load(1221-1229)load(1878-1886)load(1924-1932)load(2026-2034)load(2072-2080)load(2118-2126)load(2161-2162)load(2188-2196)load(2321-2329)load(2368-2376)load(2414-2424)delete(41-47)delete(327-331)delete(634-638)delete(812-816)delete(1231-1235)delete(1888-1892)delete(1934-1938)delete(2036-2040)delete(2082-2086)delete(2128-2132)delete(2164-2165)delete(2315-2319)delete(2362-2366)delete(2408-2412)get_url(2426-2433)get_dt(2435-2442)get_bucket(2444-2451)log_api_request(2454-2462)cla-backend-go/api_logs/models.go (1)
APILog(12-16)
cla-backend-go/api_logs/repository.go (1)
cla-backend-go/api_logs/models.go (1)
APILog(12-16)
🪛 Ruff (0.14.10)
cla-backend/cla/routes.py
57-57: Consider moving this statement to an else block
(TRY300)
58-58: Do not catch blind exception: Exception
(BLE001)
68-68: Unused function argument: response
(ARG001)
81-81: Do not catch blind exception: Exception
(BLE001)
cla-backend/cla/models/dynamo_models.py
5455-5455: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
5455-5455: Avoid specifying long messages outside the exception class
(TRY003)
5507-5507: Do not catch blind exception: Exception
(BLE001)
5514-5514: Do not catch blind exception: Exception
(BLE001)
🪛 Shellcheck (0.11.0)
utils/aws_example_api_logs_usage.sh
[error] 16-16: Couldn't parse this brace group. Fix to allow more checks.
(SC1073)
[warning] 19-19: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[error] 21-21: Expected a '}'. If you have one, try a ; or \n in front of it.
(SC1056)
[error] 21-21: Missing '}'. Fix any mentioned problems and try again.
(SC1072)
[error] 21-21: Unexpected tokens after compound command. Bad redirection or missing ;/&&/||/|?
(SC1141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (16)
cla-backend-go/api_logs/models.go (2)
11-16: LGTM - Clean data model definition.The struct definition is well-designed with appropriate types and tags for DynamoDB marshaling and JSON serialization.
18-21: LGTM - Standard String() implementation.The method provides a clear representation for debugging and logging purposes.
utils/example_scan_api_log.sh (1)
1-2: LGTM - Clear example script.The script provides a straightforward example for scanning the dev API log table. The hardcoded values are appropriate for an example/documentation script.
infra/backup-dynamodb-tables.sh (1)
63-64: LGTM - API log table added to backups.The new API log table is correctly included in the backup list with consistent naming and formatting.
cla-backend/cla/models/model_interfaces.py (1)
2388-2462: LGTM! Well-documented interface.The new APILog interface is comprehensive and follows the existing pattern in the codebase. The documentation clearly describes the expected behavior, including the three-bucket logging strategy (ALL, daily, monthly).
cla-backend/cla/routes.py (2)
44-61: Verify the broad exception handling won't mask import-time issues.The lazy import catches all exceptions, which is appropriate for a fallback mechanism. However, ensure that import failures are intentional (e.g., when the model isn't deployed yet) rather than masking actual errors like missing dependencies or syntax errors.
Consider adding more specific logging to distinguish between expected vs. unexpected import failures:
try: from cla.models.dynamo_models import APILog as _APILog _APILOG_CLS = _APILog + cla.log.info("LG:api-log-model-loaded successfully") return _APILOG_CLS except Exception as e: _APILOG_IMPORT_ERROR = e - cla.log.info(f"LG:api-log-import-failed err={e}") + cla.log.warning(f"LG:api-log-import-failed err={e} type={type(e).__name__}") return NoneIf the model should always be available in your deployment, consider whether this should log at ERROR level or even fail fast during startup.
76-82: Error handling is appropriate for best-effort logging.The broad exception catch on Line 81 ensures API logging failures never break the request flow, which is the correct behavior for observability features. The static analysis warning can be safely ignored in this context.
cla-backend/cla/models/dynamo_models.py (5)
61-61: LGTM: API log model properly integrated into database lifecycle.The
APILogModelis correctly added to bothcreate_databaseanddelete_databasetable lists, ensuring the API log table is managed consistently with other tables.Also applies to: 87-87
5389-5407: LGTM: GSI properly configured for time-range queries.The
APILogBucketDTIndexcorrectly defines a global secondary index withbucketas hash key anddtas range key, enabling efficient queries for log entries within specific time buckets (ALL, daily, monthly).
5409-5427: LGTM: Table model correctly defines composite key.The
APILogModelproperly uses a composite key (urlas hash key,dtas range key) which allows multiple log entries for the same URL with different timestamps, while the GSI enables time-range queries.
5447-5449: Verify: Commented out date_modified tracking.The
save()method hasdate_modifiedupdate commented out, which differs from the pattern used in other models that inherit fromBaseModel. If API logs don't require modification tracking for performance reasons, this is acceptable. However, if date_modified should be tracked, uncomment line 5448.Is the omission of
date_modifiedtracking intentional for API logs?
5479-5517: LGTM: Robust API request logging with defensive error handling.The
log_api_requestimplementation correctly:
- Generates millisecond timestamps and creates three log entries (ALL, daily, monthly buckets)
- Shifts timestamps by -1/0/+1 ms to avoid overwrites on the composite key (url, dt)
- Uses broad exception catching (lines 5507, 5514) which is intentionally designed to ensure logging failures never disrupt the API request flow
- Logs errors with "LG:" prefix for centralized monitoring without raising exceptions
The static analysis warnings about broad exception catching can be safely ignored as this defensive approach is appropriate for non-critical logging operations.
cla-backend-go/api_logs/repository.go (4)
17-25: LGTM: Clear interface and naming convention.The repository interface and table name constant are well-defined. The table name format matches the Python implementation.
33-39: LGTM: Constructor properly initializes repository.The
NewRepositoryconstructor correctly initializes the repository with stage and DynamoDB client, returning the interface type for better testability.
53-64: LGTM: Timestamp and bucket logic consistent with Python implementation.The timestamp generation (
UnixMilli()) and bucket creation correctly mirror the Python implementation:
- Uses millisecond precision timestamps
- Creates three entries with -1/0/+1 ms shifts to avoid overwrites
- Formats daily (YYYY-MM-DD) and monthly (YYYY-MM) buckets consistently
67-93: LGTM: Error aggregation enables partial success.The error handling strategy is well-designed:
- Collects errors per bucket but continues processing remaining entries
- Allows partial success (if "ALL" bucket fails, daily/monthly may still succeed)
- Returns aggregated error string for middleware logging
- Consistent with Python implementation's error handling philosophy
This approach is appropriate for non-critical logging where partial success is preferable to complete failure.
There was a problem hiding this comment.
Pull request overview
This PR adds API request logging functionality to DynamoDB for the EasyCLA backend services. The implementation creates a new table cla-{stage}-api-log that records every API call with timestamp and bucketing information for efficient querying.
- Adds DynamoDB table structure with url/dt primary key and bucket/dt GSI for time-range queries
- Implements middleware in both Python and Go backends to log all API requests
- Provides utility scripts for table creation, policy updates, and data querying
- Creates three log entries per request (ALL, daily, monthly buckets) with microsecond timestamp offsets to avoid key conflicts
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/example_scan_api_log.sh | Simple script to scan and display API log entries for development/testing |
| utils/aws_example_api_logs_usage.sh | Example AWS CLI commands demonstrating table operations and GSI queries |
| utils/aws_edit_table_access.sh | Script to update IAM policies to grant Lambda roles access to the new API log table |
| utils/aws_create_api_logs_table.sh | Table creation script with primary key and GSI setup |
| infra/backup-dynamodb-tables.sh | Adds the new API log table to the backup table list |
| cla-backend/cla/routes.py | Adds API logging to request middleware with lazy import to avoid circular dependencies |
| cla-backend/cla/models/model_interfaces.py | Defines the APILog interface with standard CRUD operations and log_api_request class method |
| cla-backend/cla/models/dynamo_models.py | Implements APILogModel and APILog with bucket-dt GSI and three-entry logging logic |
| cla-backend-go/cmd/server.go | Integrates API logging middleware into the Go backend server with 300ms timeout |
| cla-backend-go/api_logs/repository.go | Go implementation of the API logging repository with context-aware DynamoDB writes |
| cla-backend-go/api_logs/models.go | Defines the APILog struct with DynamoDB attribute mappings |
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cla-backend/cla/routes.py (1)
44-61: LGTM: Lazy import pattern is sound for best-effort logging.The caching mechanism prevents repeated imports while gracefully handling failures. The broad exception catch is intentional—API logging should never break the request flow.
There's a minor race condition where multiple threads could simultaneously attempt the initial import, but the impact is negligible (wasteful but not harmful, and only occurs during startup). After the first successful import, all subsequent requests use the cached value.
🔒 Optional: Add threading.Lock for perfect thread-safety
If you want to eliminate the startup race condition entirely:
+import threading + +_APILOG_CLS = None +_APILOG_IMPORT_ERROR = None +_apilog_lock = threading.Lock() def _get_apilog_cls(): """ Lazy, cached import to avoid per-request imports while staying safe against circular-import/startup ordering issues. """ global _APILOG_CLS, _APILOG_IMPORT_ERROR if _APILOG_CLS is not None: return _APILOG_CLS if _APILOG_IMPORT_ERROR is not None: return None + with _apilog_lock: + # Double-check after acquiring lock + if _APILOG_CLS is not None: + return _APILOG_CLS + if _APILOG_IMPORT_ERROR is not None: + return None try: from cla.models.dynamo_models import APILog as _APILog _APILOG_CLS = _APILog return _APILOG_CLS except Exception as e: _APILOG_IMPORT_ERROR = e cla.log.info(f"LG:api-log-import-failed err={e}") return None
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cla-backend-go/cmd/server.gocla-backend/cla/routes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cla-backend-go/cmd/server.go
🧰 Additional context used
🧬 Code graph analysis (1)
cla-backend/cla/routes.py (2)
cla-backend/cla/models/model_interfaces.py (2)
APILog(2388-2462)log_api_request(2454-2462)cla-backend/cla/models/dynamo_models.py (2)
APILog(5429-5516)log_api_request(5480-5516)
🪛 Ruff (0.14.10)
cla-backend/cla/routes.py
57-57: Consider moving this statement to an else block
(TRY300)
58-58: Do not catch blind exception: Exception
(BLE001)
68-68: Unused function argument: response
(ARG001)
84-84: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (2)
cla-backend/cla/routes.py (2)
41-42: LGTM: Clean caching globals for lazy import pattern.The module-level variables are well-named with the private convention and serve their caching purpose clearly.
68-86: LGTM: Middleware correctly integrates API logging as a best-effort operation.The implementation properly:
- Uses the lazy-import pattern to avoid per-request overhead
- Catches and logs failures to prevent API logging errors from breaking request flow
- Preserves existing GitHub activity stream behavior
- Documents the dual purpose in the docstring
The static analysis warnings are false positives:
- The
responseparameter is required by the hug framework's request middleware signature- The broad exception catch is intentional—this ensures logging never disrupts normal request handling
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cla-backend-go/api_logs/repository.go (1)
60-87: Consider using BatchWriteItem for better performance.The current implementation makes 3 separate
PutItemcalls per API request (ALL, daily, and monthly buckets). This approach:
- Triples the number of DynamoDB write operations
- Increases write latency (3 sequential network calls)
- Increases DynamoDB write costs
Consider refactoring to use
BatchWriteItemWithContextto write all three entries in a single call, which would reduce both latency and cost.♻️ Proposed optimization using BatchWriteItem
- var errs []string - for _, logEntry := range entries { - // Convert to DynamoDB attribute value - av, err := dynamodbattribute.MarshalMap(logEntry) - if err != nil { - errs = append(errs, fmt.Sprintf("bucket=%s marshal=%v", logEntry.Bucket, err)) - continue - } - - // Put item to DynamoDB - input := &dynamodb.PutItemInput{ - TableName: aws.String(tableName), - Item: av, - } - - _, err = r.dynamoDBClient.PutItemWithContext(ctx, input) - if err != nil { - errs = append(errs, fmt.Sprintf("bucket=%s put=%v", logEntry.Bucket, err)) - continue - } - } + // Prepare batch write request + writeRequests := make([]*dynamodb.WriteRequest, 0, len(entries)) + for _, logEntry := range entries { + av, err := dynamodbattribute.MarshalMap(logEntry) + if err != nil { + return fmt.Errorf("marshal bucket=%s: %v", logEntry.Bucket, err) + } + writeRequests = append(writeRequests, &dynamodb.WriteRequest{ + PutRequest: &dynamodb.PutRequest{Item: av}, + }) + } + + // Single batch write for all entries + input := &dynamodb.BatchWriteItemInput{ + RequestItems: map[string][]*dynamodb.WriteRequest{ + tableName: writeRequests, + }, + } + + _, err := r.dynamoDBClient.BatchWriteItemWithContext(ctx, input) + if err != nil { + return fmt.Errorf("batch write failed: %v", err) + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cla-backend-go/api_logs/repository.gocla-backend-go/cmd/server.go
🧰 Additional context used
🧬 Code graph analysis (2)
cla-backend-go/cmd/server.go (1)
cla-backend-go/api_logs/repository.go (2)
Repository(23-25)NewRepository(34-39)
cla-backend-go/api_logs/repository.go (1)
cla-backend-go/api_logs/models.go (1)
APILog(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress-functional
- GitHub Check: build-test-lint
🔇 Additional comments (5)
cla-backend-go/cmd/server.go (3)
18-18: LGTM: Required imports added.The
timeandapi_logsimports are necessary for the new DynamoDB logging middleware.Also applies to: 82-82
307-307: LGTM: Repository initialization.The API logs repository is correctly initialized with the stage and DynamoDB client.
433-433: LGTM: Middleware wiring.The middleware is correctly wired into the handler chain with the API logs repository.
cla-backend-go/api_logs/repository.go (2)
1-20: LGTM: Package setup and table name constant.The imports and table name constant are correctly defined for DynamoDB operations.
22-39: LGTM: Interface and constructor design.Clean separation of interface and implementation with proper encapsulation.
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cla-backend-go/cmd/server.go:
- Line 308: The code instantiates apiLogsRepo via api_logs.NewRepository(stage,
dynamodb.New(awsSession)) but the serverless IAM policy is missing permissions
for the DynamoDB table cla-${opt:stage}-api-log; add the table ARN
"arn:aws:dynamodb:${self:custom.dynamodb.region}:${aws:accountId}:table/cla-${opt:stage}-api-log"
to the IAM policy in serverless.yml and ensure the cla-dev-api-log and
cla-prod-api-log tables are created in your infra with the required schema
(partition key: url, sort key: dt) so LogAPIRequest and apiLogsRepo writes
succeed at runtime.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cla-backend-go/cmd/server.go
🧰 Additional context used
🧬 Code graph analysis (1)
cla-backend-go/cmd/server.go (1)
cla-backend-go/api_logs/repository.go (2)
Repository(23-25)NewRepository(34-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (3)
cla-backend-go/cmd/server.go (3)
18-18: LGTM: Import additions are appropriate.The new imports (
timeandapi_logs) are necessary for the DynamoDB logging middleware functionality.Also applies to: 82-82
149-177: Excellent implementation: Previous review feedback fully addressed.The async DynamoDB logging middleware correctly implements the recommendations from the previous review:
✅ Timeout increased: Now 3 seconds (line 165), exceeding the suggested 1-2s minimum
✅ Async execution: Runs in a goroutine (line 158) to avoid blocking the request
✅ Panic recovery: Properly wraps the goroutine body (lines 159-163)
✅ Correct context: Usescontext.Background()instead of request context (line 165)
✅ Closure safety: Path variable captured before goroutine (line 157)The fire-and-forget pattern is appropriate for logging—errors are logged but never fail the request.
434-434: LGTM: Middleware wiring is correct.The new DB-backed logger is properly integrated as the outermost middleware in the chain, ensuring all API requests are logged before any other processing occurs. The repository is correctly passed to the factory function.
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cla-backend-go/serverless.yml (1)
125-125: Consider scoping permissions to logging requirements.The API log table inherits broad DynamoDB permissions including
UpdateItemandDeleteItem. If the table is write-only for logging purposes, consider creating a separate, narrower policy statement that grants onlyPutItemandQueryto reduce the attack surface.cla-backend/serverless.yml (1)
210-210: Consider scoping permissions to logging requirements.Similar to the Go backend, the API log table inherits broad DynamoDB permissions including
UpdateItemandDeleteItem. If the table is primarily for write-only logging, consider a separate policy statement with narrower permissions (PutItemandQueryonly).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cla-backend-go/serverless.ymlcla-backend/serverless.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (2)
cla-backend-go/serverless.yml (1)
148-148: LGTM!The GSI ARN is correctly added to the Query-only permission block, which aligns with DynamoDB best practices since indexes don't support direct write operations.
cla-backend/serverless.yml (1)
233-233: LGTM!The GSI ARN is correctly placed in the Query-only permission block, consistent with DynamoDB index access patterns and matching the Go backend configuration.
This adds logging of API calls to a new DynamoDB table
cla-{stage}-api-log.This table (and its GSI index allowing date range search) were added on both
devandprodbut for lambdas to use them (add entries on API call) a policy update is needed. We need to list this additional table and its GSI index in 2 policies per environment (dev/prod):I've added them in
{stage}=devpolicy, but I can't do this forprodbecause I don't have permissions. So we can deploy this todevand test but forprodwe need to update the following policies:but adding this diff to both policies (marked with
+at the line beginning:I'm getting:
cc @mlehotskylf @jarias-lfx
This should (?) also be replicated to Snowflake (automatically? Or do I need to update something?) so we will be able to generate SQL reports based on this as well.