Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UNOMI-846: improve log for event validation #687

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Conversation

jsinovassin
Copy link
Contributor

@jsinovassin jsinovassin marked this pull request as draft August 22, 2024 15:47
@jsinovassin jsinovassin marked this pull request as ready for review October 8, 2024 10:34
Copy link
Contributor

@jkevan jkevan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the fact that I think changing this log levels directy in the code is a bad idea, mostly because of:

  • security
  • amount of log printed by default
  • the fact that normally log level is configurable (should be configurable) outside the code.

There is inconsistent logs, I wonder why sometimes for a ValidationException we now sometimes log it as:

 LOGGER.warn("An event was not valid - switch to DEBUG log level for more information.");
            LOGGER.debug("{}", e.getMessage(), e);

and sometimes like this:

 LOGGER.warn("{}", e.getMessage(), e);

Also I am not really sure about the wording in the message: "An event was not valid" in case of ValidationException, to me it's more a crash/error before/during the validation. Which is different than schema validation results.
I would rename the log to: "An error occurred during event validation", to make that difference, if the goal of thoses logs are to be clear for developers to have useful/understandable informations, they should be clear and consistent.

@@ -226,9 +228,9 @@ private Set<ValidationError> validate(JsonNode jsonNode, JsonSchema jsonSchema)
Set<ValidationMessage> validationMessages = jsonSchema.validate(jsonNode);

if (LOGGER.isDebugEnabled() && !validationMessages.isEmpty()) {
LOGGER.debug("Schema validation found {} errors while validating against schema: {}", validationMessages.size(), jsonSchema.getCurrentUri());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOGGER.isDebugEnabled() should be removed

@jsinovassin jsinovassin force-pushed the UNOMI-846 branch 2 times, most recently from 68e0c02 to 813211a Compare October 14, 2024 09:05
@jsinovassin jsinovassin merged commit 64aa2a8 into master Oct 14, 2024
5 checks passed
@jsinovassin jsinovassin deleted the UNOMI-846 branch October 14, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants