-
Notifications
You must be signed in to change notification settings - Fork 3
Add: logs #124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add: logs #124
Conversation
WalkthroughAdds project-scoped debug logging to the Appwrite migration source and its Database reader. Introduces a public static array Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Migration/Sources/Appwrite.php (2)
13-13: Hard‑coded debug project map and new Console dependencyThe
Utopia\CLI\Consoleimport pluspublic static array $debugProjectslook intentionally tailored to a couple of specific projects. That’s fine for targeted troubleshooting, but it does bake environment‑specific state into the main source:
- If this is meant to be long‑term, consider moving the mapping to configuration (env, CLI flag, or injected array) so you don’t need a code change to add/remove projects.
- If this is short‑term debugging, it might be safer to keep it clearly marked as such (e.g. a comment noting it should be cleaned up once incidents are resolved).
The public visibility is required for
DatabaseReaderto access it, but reinforces that this becomes part of the public surface ofAppwrite. If you want to keep it semi‑internal, a dedicated accessor (e.g.Appwrite::getDebugTagForProject(string $id): ?string) would decouple the reader from the concrete static property.Also applies to: 78-82
992-1127: Verbose per‑record logging inexportRecordsis well‑gated but very chattyThe new logging in
exportRecords()(entity start/end, per‑table start/end, per‑iteration memory usage, timestamps aroundlistRows(), and before/aftercallback()) is implemented safely:
- It doesn’t affect control flow or data transformation.
- It’s all routed through
logDebugTrackedProject(), so only projects present in$debugProjectspay the cost.Two considerations:
- For the tracked projects, this can generate very large log volumes on big tables (every iteration, every batch). Make sure your log sink/rotation can handle that or consider coarse‑graining (e.g. log every Nth iteration, or only when memory crosses thresholds).
- If you expect to reuse this style of instrumentation later, it might be worth extracting the message formatting into a tiny helper (or at least centralizing the prefixes) to reduce duplication and make it easier to tweak the format.
Functionally, the loop logic and pagination behavior remain intact.
src/Migration/Sources/Appwrite/Reader/Database.php (1)
5-6: Project‑aware debug logging in reader + CI/phpstan implicationsThe changes here (new imports, extended constructor, and
logDebug()helper) cleanly introduce project‑scoped logging:
?string $projectIdis optional and readonly, so existing call sites that don’t pass it remain valid.logDebug()is a no‑op unlessprojectIdis non‑null and present inAppwrite::$debugProjects, so there’s effectively zero runtime overhead for non‑tracked projects.- Using
Appwrite::$debugProjectskeeps tags consistent with the source side.Things to watch:
- Tight coupling to
Appwrite:Databasenow depends onUtopia\Migration\Sources\Appwritepurely for debug metadata. If you ever want to reuse this reader in another context, it may be nicer to inject a genericcallableor an interface for lookup instead of referencing the source class directly.- phpstan “unknown class Console”: As with
Appwrite.php, CI reportsCall to static method log() on an unknown class Utopia\CLI\Console.This will keep blocking the pipeline until phpstan is taught about theutopia-php/clipackage or these lines are stubbed/ignored in analysis.Overall, the constructor change and helper are backward‑compatible and safe; the only blocker is static analysis configuration around
Console.Also applies to: 19-20, 32-38, 40-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Migration/Sources/Appwrite.php(7 hunks)src/Migration/Sources/Appwrite/Reader/Database.php(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
Repo: utopia-php/migration PR: 0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.
Applied to files:
src/Migration/Sources/Appwrite.phpsrc/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.php
🧬 Code graph analysis (2)
src/Migration/Sources/Appwrite.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
listRows(334-387)src/Migration/Sources/Appwrite/Reader/API.php (1)
listRows(179-187)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
src/Migration/Sources/Appwrite.php (1)
Appwrite(73-1968)
🪛 GitHub Actions: CodeQL
src/Migration/Sources/Appwrite.php
[error] 151-151: Call to static method info() on an unknown class Utopia\CLI\Console. (phpstan)
src/Migration/Sources/Appwrite/Reader/Database.php
[error] 54-54: Call to static method log() on an unknown class Utopia\CLI\Console. (phpstan)
🔇 Additional comments (1)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
334-387: Additional logging aroundlistRows()is safe and consistentThe new calls:
$this->logDebug("BEFORE dbInstance->find() | TableID: $tableId | Queries: " . count($queries)); $rows = $dbInstance->find($tableId, $queries); $this->logDebug("AFTER dbInstance->find() | TableID: $tableId | Rows returned: " . count($rows));are:
- Correctly placed around the actual
find()call.- Helpful for correlating source‑side export logs with reader‑side query behavior.
- Fully gated by
logDebug(), so they’re effectively free outside tracked projects.No functional behavior changes are introduced; the existing error handling and row mapping logic are preserved.
Also applies to: 369-373
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)
992-1127: Comprehensive instrumentation with minor performance consideration.The extensive logging provides excellent visibility into the export process with well-placed checkpoints around critical operations (
listRows(),callback()). The inclusion of memory usage, timestamps, and iteration counts will be valuable for debugging.Since this logging is opt-in (only for projects in
$debugProjects), the performance impact is limited to explicitly tracked migrations. Just ensure that$debugProjectsremains a small set in production environments to avoid I/O overhead from frequent logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Migration/Sources/Appwrite.php(7 hunks)src/Migration/Sources/Appwrite/Reader/Database.php(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
Repo: utopia-php/migration PR: 0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.
Applied to files:
src/Migration/Sources/Appwrite.phpsrc/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.
Applied to files:
src/Migration/Sources/Appwrite/Reader/Database.php
🧬 Code graph analysis (1)
src/Migration/Sources/Appwrite.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
listRows(334-387)src/Migration/Sources/Appwrite/Reader/API.php (1)
listRows(179-187)
🔇 Additional comments (7)
src/Migration/Sources/Appwrite/Reader/Database.php (4)
5-5: LGTM!The imports are correctly added to support the new logging functionality.
Also applies to: 19-19
32-38: LGTM!The optional
$projectIdparameter is well-designed with appropriate visibility and immutability constraints, maintaining backward compatibility.
40-55: LGTM!The logging implementation is clean with efficient early returns. The consistent use of
Console::info()across both files addresses the concern raised in previous reviews about method consistency.
369-372: LGTM!The before/after instrumentation around
dbInstance->find()provides valuable debugging visibility with appropriate context (table ID, query count, row count).src/Migration/Sources/Appwrite.php (3)
13-13: LGTM!The Console import is correctly added to support the new logging functionality.
128-132: LGTM!The DatabaseReader construction correctly passes the project ID to enable project-scoped logging, maintaining consistency with the updated reader interface.
141-152: LGTM!The logging helper is well-implemented with efficient early returns and consistent Console method usage. The distinct tag prefix (
MIGRATIONS-SOURCE-vsMIGRATIONS-READER-) helps differentiate log sources.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.