Use context aware logger for workflow service #5240
Merged
Conversation
This file contains 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
The workflow service is using a special logger that supports units of work so that you can identify several logger statements belonging to a specific task. However, with the new implementation, there is but one unit of work definition left, and it contains just a single logging statement. Nothing you actually still need this for. ``` logger.startUnitOfWork(); logger.warn(failureToFail, "Unable to update workflow to failed state"); logger.endUnitOfWork(); ``` The downside to using this special logger is that you loose context information. With the normal logger you get information about the location where this has been logged. With the special logger this information is lost. That is really annoying when debugging based on logs: ``` 2023-09-05 18:14:57,449 | INFO | (ServiceRegistryJpaImpl:622) - Jobs with IDs '[8579450]' deleted 2023-09-05 18:14:57,450 | WARN | (Log:213) - [>2ef22a08] No events for workflow instance 8579450 found in the Elasticsearch index. 2023-09-05 18:14:57,496 | DEBUG | (Log:213) - [>2ef22a08] Deleting workflow instance 8579614 2023-09-05 18:14:57,548 | INFO | (Log:213) - [>2ef22a08] Removing temporary files for workflow 8579614 2023-09-05 18:14:57,548 | DEBUG | (Log:213) - [>2ef22a08] Removing temporary file https://oc-admin.virtuos.uni-osnabrueck.de/files/mediapackage/6a092362-56c4-46a9-b82d-f389d97ca19c/security-policy-series/xacml.xml for workflow Workflow {8579614} ``` Given that it is doing more harm than good, this patch switches the workflow service to the default logger. With this you also now get proper context information about logs: ``` 2023-09-05 18:43:34,069 | INFO | (ServiceRegistryJpaImpl:622) - Jobs with IDs '[9256488]' deleted 2023-09-05 18:43:34,073 | WARN | (WorkflowServiceImpl:2260) - No events for workflow instance 9256488 found in the Elasticsearch index. 2023-09-05 18:43:34,082 | DEBUG | (WorkflowServiceImpl:2127) - Deleting workflow instance 9256489 2023-09-05 18:43:34,107 | INFO | (WorkflowServiceImpl:881) - Removing temporary files for workflow 9256489 ```
Arnei
approved these changes
Sep 7, 2023
There was a problem hiding this comment.
More context information is good, approved.
If me IDE isn't lying to me, this PR removes the last file where the startUnitOfWork() and endUnitOfWork() methods were actually used. Does this mean we can get rid of the special logger since its special logging capabilities are no longer required, or is there a reason to keep it around? Not that this PR should do the removing, but it would be worth an issue/follow-up PR.
|
I think we can and should remove it. |
mtneug
reviewed
Sep 12, 2023
| @@ -69,7 +69,6 @@ | |||
| import org.opencastproject.serviceregistry.api.ServiceRegistry; | |||
| import org.opencastproject.serviceregistry.api.ServiceRegistryException; | |||
| import org.opencastproject.serviceregistry.api.UndispatchableJobException; | |||
| import org.opencastproject.util.Log; | |||
There was a problem hiding this comment.
I haven't checked: Is this used anywhere else? Can this whole class be removed?
There was a problem hiding this comment.
It's still used, so you can't "just" remove it. Shouldn't be too hard though. But as Lars said, that's another PR.
|
Filed #5261 about the removal of the logger |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The workflow service is using a special logger that supports units of work so that you can identify several logger statements belonging to a specific task.
However, with the new implementation, there is but one unit of work definition left, and it contains just a single logging statement. Nothing you actually still need this for.
The downside to using this special logger is that you loose context information. With the normal logger you get information about the location where this has been logged. With the special logger this information is lost. That is really annoying when debugging based on logs:
Given that it is doing more harm than good, this patch switches the workflow service to the default logger. With this you also now get proper context information about logs:
Your pull request should…