[ML][Transforms] unifying logging, adding some more logging #45788
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Pinging @elastic/ml-core |
| listener.onResponse(new StartDataFrameTransformTaskAction.Response(true)); | ||
| }, | ||
| exc -> { | ||
| logger.error("[" + transform.getId() + "] failed updating state to [" + state + "].", exc); |
Contributor
There was a problem hiding this comment.
Is it possible to use interpolation markers ("{}") here (and in other places) the same way as you did in line 355?
Member
Author
There was a problem hiding this comment.
Its not possible to use interpolation and have an exception too.
Member
There was a problem hiding this comment.
Use a ParameterizedMessage
Suggested change
| logger.error("[" + transform.getId() + "] failed updating state to [" + state + "].", exc); | |
| logger.error(new ParameterizedMessage("[{}] failed updating state to [{}].", transform.getId(), state), exc); |
Contributor
There was a problem hiding this comment.
+1
I forgot about it. Seems like a perfect solution to maintain consistency of how the messages are crafted in this file.
Member
Author
|
run elasticsearch-ci/1 |
Member
Author
|
@elasticmachine update branch |
…enwtrent/elasticsearch into feature/ml-df-checkpoint-logging-fixes
przemekwitek
approved these changes
Aug 22, 2019
Contributor
przemekwitek
left a comment
przemekwitek
left a comment
There was a problem hiding this comment.
Two minor comments, otherwise LGTM
|
|
||
| synchronized void handleFailure(Exception e) { | ||
| logger.warn("Data frame transform [" + transformTask.getTransformId() + "] encountered an exception: ", e); | ||
| logger.warn(new ParameterizedMessage("[{} data frame transform encountered an exception: ", |
| } catch (Exception e) { | ||
| logger.error("Data frame transform encountered an unexpected internal exception: " ,e); | ||
| logger.error( | ||
| new ParameterizedMessage("[{} data frame transform encountered an unexpected internal exception: ", transformId), |
benwtrent
added a commit
to benwtrent/elasticsearch
that referenced
this pull request
Aug 22, 2019
…45788) * [ML][Transforms] unifying logging, adding some more logging * using parameterizedMessage instead of string concat * fixing bracket closure
benwtrent
added a commit
that referenced
this pull request
Aug 22, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR unifies our logging messages in the transform task so that they are easier to grok. Some new logging entries are added, but one logging message is removed (it was a hold over from previous debugging efforts).
Additionally, the "when to audit on checkpoint finish" logic is now changed so that we write an audit message for nicely rounded numbers such as
10,100, and not19,99or199.