Improve ETLv2 query debug #46
Conversation
- Augment Loggable::logAndThrowException() to customize message based on optional parameters - Merge Loggable::logAndThrowSqlException() into Loggable::logAndThrowException() - Improve display of SQL in debug mode - Display DataEndpoint associated with every query now that ETL is getting more complex
smgallo
added
the
enhancement
| case 'regex': | ||
| if ( false === preg_match($directive->format, "test") ) { | ||
| $msg = "Invalid regex format '{$directive->format}' for key '$key'"; | ||
| $this->logAndThrowException($msg); |
There was a problem hiding this comment.
do we need to set the error message to a variable or should we just pass it into the function call?
| } | ||
| break; | ||
| default: | ||
| $msg = "Unsupported transform type '{$directive->type}' for key '$key'"; |
There was a problem hiding this comment.
do we need to set the error message to a variable or should we just pass it into the function call?
| $msg = "Invalid regex format '{$directive->format}' for key '$key'"; | ||
| case 'regex': | ||
| if ( false === preg_match($directive->format, "test") ) { | ||
| $msg = "Invalid regex format '{$directive->format}' for key '$key'"; |
There was a problem hiding this comment.
do we need to set the error message to a variable or should we just pass it into the function call?
| } | ||
| break; | ||
| default: | ||
| $msg = "Unsupported transform type '{$directive->type}' for key '$key'"; |
There was a problem hiding this comment.
do we need to set the error message to a variable or should we just pass it into the function call?
| $matches = null; | ||
| $matched = preg_match($directive->format, $value, $matches); | ||
| if ( false === $matched ) { | ||
| $msg = "Error transforming regex '{$directive->format}'"; |
There was a problem hiding this comment.
do we need to set the error message to a variable or should we just pass it into the function call?
| case 'regex': | ||
| $matched = preg_match($directive->format, $value); | ||
| if ( 0 === $matched ) { | ||
| $msg = "Failed {$directive->type} ({$directive->format}) verification for '$value'"; |
There was a problem hiding this comment.
do we need to set the error message to a variable or should we just pass it into the function call?
| $sql = "UPDATE " . $this->etlDestinationTable->getFullName() . " SET " | ||
| . implode( | ||
| ", ", | ||
| array_map( |
There was a problem hiding this comment.
could/should we turn these two into a function? pass the connector (', ' or ' AND ' ) and the values?
|
Addressed @plessbd review comments. The change to UpdateIngestor.php is out of the scope of this pull request but there is a plan to deal with that by moving query construction tools into their own utility class. |
* Make all aggregation period data available as bind parameters to query * Fix phpcs issues * Addressed code review comments * Improve ETLv2 debugging messages - Augment Loggable::logAndThrowException() to customize message based on optional parameters - Merge Loggable::logAndThrowSqlException() into Loggable::logAndThrowException() - Improve display of SQL in debug mode - Display DataEndpoint associated with every query now that ETL is getting more complex * Fix phpcs issues * Address @plessb code review comments
Improve ETLv2 query debuging
Description
Augment Loggable::logAndThrowException() to accept a list of options and customize messages based on these options. This allowed us to merge Loggable::logAndThrowSqlException() into Loggable::logAndThrowException() and improve the display of SQL in debug mode.
Also display the DataEndpoint associated with every query now and fix phpcs style issues.
Note that commits 216aa24, 8bfbcac, 729b1da were from a local merge and have already been merged in #45.
Motivation and Context
ETL is getting more complex pulling data from multiple databases, schemas, and files. It is important when debugging to know the queries being generated and the databases and schemas (DataEndpoints) that they are being run against.
Tests performed
Ran the following ETLv2 pipelines: xdcdb-jobs (with and without aggregation binning), test-suite, resource-allocations. These test the following actions:
Types of changes
Checklist: