feat: Add CDF tracing for Phase 1 of Change Data feed #1654
Conversation
…kernel-rs into table-config-tracing
…kernel-rs into table-config-tracing
…kernel-rs into table-config-tracing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1654 +/- ##
==========================================
- Coverage 84.63% 84.63% -0.01%
==========================================
Files 125 125
Lines 34724 34745 +21
Branches 34724 34745 +21
==========================================
+ Hits 29390 29405 +15
- Misses 3983 3989 +6
Partials 1351 1351 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let logs = Arc::new(Mutex::new(Vec::new())); | ||
| let logs_clone = logs.clone(); | ||
|
|
||
| let subscriber = tracing_subscriber::registry().with( | ||
| tracing_subscriber::fmt::layer() | ||
| .with_writer(move || LogWriter(logs_clone.clone())) | ||
| .with_ansi(false), | ||
| ); | ||
|
|
||
| let _guard = tracing::subscriber::set_default(subscriber); |
There was a problem hiding this comment.
I'm also noticing that we're duplicating this. Maybe make this a helper function for setting up a logging test?
| let commits = get_segment(engine.as_ref(), mock_table.table_root(), 0, None) | ||
| .unwrap() | ||
| .into_iter(); | ||
|
|
||
| let table_root_url = url::Url::from_directory_path(mock_table.table_root()).unwrap(); | ||
| let table_config = get_default_table_config(&table_root_url); | ||
|
|
||
| let _scan_batches: DeltaResult<Vec<_>> = | ||
| table_changes_action_iter(engine, &table_config, commits, get_schema().into(), None) | ||
| .unwrap() | ||
| .try_collect(); | ||
|
|
||
| let log_output = tracing_guard.logs(); |
There was a problem hiding this comment.
I added LoggingTest to reduce some duplication of the logging setup as suggested, but also noticed that the tests are still quite similar in structure. I thought about extracting out basically this logic to another function, but I figure that other tests for tracing might have slightly different structures and this might not be necessary if it'll only be used for these four tests that have been added. Let me know if you think I should add this though!
There was a problem hiding this comment.
yeah it's not the best. A lot of it is from the poor formatting of mock_table's stuff 😔 Since the tests in that module are generally similar, I think this is fine.
2 participants
What changes are proposed in this pull request?
fix: #1640
This PR adds tracing at the end of phase 1 of CDF to help with debugging errors.
How was this change tested?
Added a test to check that all the tracing statements are found in the logs.