Add logging flags for tuple writes #535
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change adds support for logging successful and failed tuple write and delete operations to separate files in various formats (CSV, YAML, JSON/JSONL) via new CLI flags. The logging is managed by a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant TupleLogger
participant Importer
User->>CLI: Run tuple write/delete with --success-log/--failure-log
CLI->>TupleLogger: Create TupleLogger(s) for provided log paths
CLI->>Importer: Pass context with loggers to import function
Importer->>TupleLogger: Log successes and failures during processing
Importer->>CLI: Return results
CLI-->>User: Output (suppressed if logging enabled)
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (3)
internal/tuple/logger.go (2)
63-79: Consider error handling for marshaling operations.The LogSuccess method handles multiple formats well, but ignores marshaling errors. While this might be acceptable for logging to avoid disrupting the main flow, consider at least logging marshal failures.
case ".yaml", ".yml": - b, _ := yaml.Marshal(key) + b, err := yaml.Marshal(key) + if err != nil { + // Could log marshal error or use a fallback + return + } l.writer.Write(b)
82-98: Same marshaling concern applies to LogFailure.Similar to LogSuccess, consider handling marshaling errors more explicitly to improve robustness.
internal/tuple/import.go (1)
330-337: Consider consistency in variable naming.The variable
failedis used in bothprocessWritesandprocessDeletesfunctions. Consider using more descriptive names likefailedWriteandfailedDeleteto improve code clarity, especially since these are in different contexts.# In processWrites function: - failed := failedWriteResponse{ + failedWrite := failedWriteResponse{ TupleKey: write.TupleKey, Reason: reason, } - failedWrites = append(failedWrites, failed) + failedWrites = append(failedWrites, failedWrite) if failureLogger != nil { - failureLogger.LogFailure(failed) + failureLogger.LogFailure(failedWrite) } # In processDeletes function: - failed := failedWriteResponse{ + failedDelete := failedWriteResponse{ TupleKey: deletedTupleKey, Reason: reason, } - failedDeletes = append(failedDeletes, failed) + failedDeletes = append(failedDeletes, failedDelete) if failureLogger != nil { - failureLogger.LogFailure(failed) + failureLogger.LogFailure(failedDelete) }Also applies to: 370-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/tuple/delete.go(3 hunks)cmd/tuple/tuple.go(1 hunks)cmd/tuple/write.go(3 hunks)internal/tuple/import.go(4 hunks)internal/tuple/import_test.go(3 hunks)internal/tuple/logger.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/tuple/delete.go (2)
internal/tuple/logger.go (4)
TupleLogger(18-24)NewTupleLogger(27-46)WithSuccessLogger(121-123)WithFailureLogger(125-127)internal/tuple/import.go (1)
ImportTuplesWithoutRampUp(85-90)
cmd/tuple/write.go (1)
internal/tuple/logger.go (4)
TupleLogger(18-24)NewTupleLogger(27-46)WithSuccessLogger(121-123)WithFailureLogger(125-127)
⏰ 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). (5)
- GitHub Check: Tests
- GitHub Check: Test Release Process
- GitHub Check: Lints
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
🔇 Additional comments (17)
cmd/tuple/tuple.go (1)
41-42: LGTM! Clean flag implementation.The new persistent flags for success and failure logging are well-defined and follow the existing pattern. The descriptive names and help text make their purpose clear.
internal/tuple/import_test.go (3)
4-4: LGTM! Appropriate context import.The context import is necessary for the updated function signatures.
27-27: LGTM! Correct context usage in tests.Using
context.Background()is the appropriate choice for unit tests where no specific context behavior is needed.
49-49: LGTM! Consistent context usage.The context parameter is consistently applied to both test functions.
cmd/tuple/delete.go (3)
52-68: LGTM! Proper logger setup and resource management.The logger creation follows good practices:
- Proper error handling for logger instantiation
- Appropriate resource cleanup with deferred Close() calls
- Conditional creation based on flag values
The ignored errors from
flag.GetString()are acceptable since these are optional flags that won't fail parsing.
90-93: LGTM! Clean context propagation.The loggers are properly attached to the context and passed to the import function, enabling logging throughout the tuple processing pipeline.
105-111: LGTM! Smart output suppression logic.The conditional output suppression prevents duplicate information when logging is enabled, improving user experience by avoiding redundant console output.
cmd/tuple/write.go (3)
236-254: LGTM! Consistent logger implementation.The logger setup follows the same excellent pattern as in the delete command:
- Proper conditional creation based on flag values
- Good error handling for logger instantiation
- Appropriate resource cleanup with deferred Close() calls
Consistency across commands enhances maintainability.
266-267: LGTM! Proper context setup.The loggers are correctly attached to the context, enabling the logging functionality throughout the import process.
282-288: LGTM! Thoughtful output management.The conditional suppression of successful and failed tuples in the console output when logging is enabled prevents information duplication and improves the user experience.
internal/tuple/logger.go (4)
27-46: LGTM! Robust file handling and logger initialization.The constructor properly handles file creation, permission setting (0o600 is appropriate for log files), and tracks existing file size to determine header status. Error handling is comprehensive with proper resource cleanup.
48-60: LGTM! Proper resource cleanup and data persistence.The Close and flush methods ensure data integrity by flushing buffers and syncing to disk. The nil check in Close prevents panics.
116-137: LGTM! Clean context utilities implementation.The context key approach using a custom type prevents key collisions, and the getter functions safely handle type assertions. This provides a clean API for logger injection and retrieval.
100-112: CSV escaping is handled by encoding/csv — no action neededGo’s
encoding/csv.Writerautomatically follows RFC 4180, quoting fields containing commas, quotes, or newlines and doubling any embedded quotes. Your use ofcsv.NewWriterwith a simpleWrite(...)call correctly handles all necessary escaping.internal/tuple/import.go (3)
143-143: Context parameter integration looks consistent.The context parameter has been properly added to all the process functions and their call sites. The changes maintain consistency across the codebase.
Also applies to: 218-219, 301-301, 304-305, 311-311
325-327: Logging integration follows good practices.The conditional logging implementation is well-structured:
- Loggers are checked for nil before use
- Success and failure cases are handled appropriately
- The logging doesn't interfere with the main business logic
The pattern of creating the
failedvariable before appending and logging (lines 330-337, 370-377) is a good refactor that makes the code more readable.Also applies to: 335-337, 365-367, 375-377
319-320: Logger helpers are defined in internal/tuple/logger.go
The functionsgetSuccessLoggerandgetFailureLoggerexist at lines 129–137 ininternal/tuple/logger.go. No further changes are needed here.
There was a problem hiding this comment.
Code looks good - in the morning, I'd like to test it quickly before merging.
Later, for us: we should create a ticket to consolidate our outputs, we write yaml ans csv and json in various places.
Having an output manager that deals with this would be a good idea
|
@rhamzeh the main issue with this approach is that we can't know why a tuple failed to write. Maybe we need to output a "reason" field on the error logs and ignore that field when importing.. |
Summary
Testing
make test-unitgo build ./...https://chatgpt.com/codex/tasks/task_e_686c2ec5a4b48322adc0e1ba7263f18e
Summary by CodeRabbit
New Features
Documentation