[PECOBLR-335] Unify logging + telemetry across the repo#812
Conversation
samikshya-db
changed the title
| String errorMessage = | ||
| String.format("Error converting element at index %s: %s", i, e.getMessage()); | ||
| LOGGER.error(errorMessage, e); | ||
| LOGGER.error(e, errorMessage); |
There was a problem hiding this comment.
At multiple places, like this, the string.format is not inline, what should we do about this to take advantage of lazy eval?
There was a problem hiding this comment.
At multiple places
No, only in case of errors. This is because we anyway need the string for throwing an exception.
| @Override | ||
| public void setLoginTimeout(int seconds) { | ||
| LOGGER.debug(String.format("public void setLoginTimeout(int seconds = {%s})", seconds)); | ||
| LOGGER.debug("public void setLoginTimeout(int seconds = {})", seconds); |
There was a problem hiding this comment.
I don't think this will work, JDBC uses JUL by default and that does not recognise {} so it will just add {} to the log
There was a problem hiding this comment.
I don't think this will work,
How so?
I have tested this locally as well as the logging integration test works. I have also changed the JUL implementation in this PR, can you take a look?
There was a problem hiding this comment.
have also changed the JUL implementation in this PR
oh sorry missed this, will re-review
| return DEFAULT_PACKAGE_PREFIX; | ||
| } | ||
|
|
||
| private String slf4jToJavaFormat(String format) { |
| String stackTraceMessage = | ||
| format( | ||
| "DatabricksResultSet executeInternal(String sql = %s,Map<Integer, ImmutableSqlParameter> params = {%s}, StatementType statementType = {%s})", | ||
| "DatabricksResultSet executeInternal(String sql = %s, Map<Integer, ImmutableSqlParameter> params = {%s}, StatementType statementType = {%s})", |
There was a problem hiding this comment.
do we need {%s} here?
There was a problem hiding this comment.
%s is required as we are using the String.format method (because it is an exception error message)
There was a problem hiding this comment.
yeah, i guess my question was do we need the braces since we changed {%s} to {} at certain places in this PR.. so do we want to change {%s} to just %s here
There was a problem hiding this comment.
Either works, as long as we have the string required.
samikshya-db
changed the title
samikshya-db
had a problem deploying
to
azure-prod
GitHub Actions
Failure
3 participants
Description
Log(String.format(""...%s", stringText)-> this is because SLF4J inbuilt interpolation of string lazily does the job (i.e., interpolates only if specified log level is on + log is passed) -> hence is more performant.Testing
Additional Notes to the Reviewer