Improve failure logging #207
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
It's important users are able to debug their process properly if a test fails. To enable this we should provide them with proper logging upon a failure. In the past we used the CompactRecordLogger from zeebe-test-util for this. This solution had 2 problems: 1. zeebe-test-util is not bound to Java 8. This means users running on Java 8 cannot use it. 2. The CompactRecordLogger provides too much information for the targeted users of this project. To solve these problems I have introduced a new logger. This will also log all records of the stream, but only logs the most important fields that users need for debugging their processes. At the moment it is a guess which fields these are. Once we receive user feedback we can easily modify the logging for the specific records that are not clear enough.
We shouldn't use System.out logging, but instead use a proper logger.
remcowesterhoud
force-pushed
the
188-failure_logging
branch
from
2cf0a25
to
4544eb0
Compare
korthout
requested changes
Feb 24, 2022
There was a problem hiding this comment.
I really like these changes @remcowesterhoud Great job! 👍
❌ I have a few improvements we should consider before merging. Mainly, I want to avoid confusing users unnecessarily.
filters/src/main/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLogger.java
Outdated
Show resolved
Hide resolved
filters/src/main/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLogger.java
Outdated
Show resolved
Hide resolved
extension/src/main/java/io/camunda/zeebe/process/test/extension/ZeebeProcessTestExtension.java
Outdated
Show resolved
Hide resolved
We always call #logGenericRecord for any record. If we haven't defined the value type in the constructor we would call #logGenericRecord a second time, thus logging the record twice. If we haven't defined the value type we shouldn't log any extra information.
Adding new records to this project is something that will easily be forgotten. By adding a test we can guarantee that all Records have been accounted for.
We logged all incidents. This also included all incidents that were resolved. If the user resolved an incident then it's safe to assume they are aware of the it, and we don't need to log it anymore. With this change we only log the incidents that have not yet been resolved.
Some record types, value types and intents did not fit in the dedicated space reserved for it. The largest RecordType (COMMAND_REJECTION) is 17 characters. This fits in the 20 character range. The largest ValueType (MESSAGE_START_EVENT_SUBSCRIPTION) is 32 characters. This fits in the 35 character range. The larges Intent (CREATE_WITH_AWAITING_RESULT) is 27 characters. This fits in the 30 character range.
|
@korthout Thanks a lot for all the reviews you're doing for me! 🏆 You raised some valid points here and I've made the necessary changes. Please have another look 🙇 |
korthout
approved these changes
Feb 24, 2022
There was a problem hiding this comment.
Nice work @remcowesterhoud 🦖
🔑 I have one last change request, but I already want to provide my approval so this isn't blocked by me anymore. So here's a key to represent me handing this over 😆
filters/src/main/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLogger.java
Outdated
Show resolved
Hide resolved
filters/src/main/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLogger.java
Show resolved
Hide resolved
filters/src/main/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLogger.java
Outdated
Show resolved
Hide resolved
The RecordStreamLogger also took care of printing any incidents that occurred. It's fitting to put this responsibility into a separate logger.
With this change we general text surrounding the logging has been improved. This is done to give the users some more detailed information about what could be wrong and where to look to fix it.
remcowesterhoud
force-pushed
the
188-failure_logging
branch
from
f87d84a
to
954bc99
Compare
Renamed value loggers from log*Record to log*RecordValue. This makes it more clear that we are logging the value of the record in these functions.
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.
Description
It's important users are able to debug their process properly if a test fails. To enable this we should provide them with proper logging upon a failure.
In the past we used the CompactRecordLogger from zeebe-test-util for this. This solution had 2 problems:
To solve these problems I have introduced a new logger. This will also log all records of the stream, but only logs the most important fields that users need for debugging their processes. At the moment it is a guess which fields these are. Once we receive user feedback we can easily modify the logging for the specific records that are not clear enough.
Related issues
closes #188
closes #175
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: