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

fix: changes logging for skipped validators to atInfo instead of atSevere in printSummary #2016

Merged
merged 2 commits into from
Mar 28, 2025

Conversation

eibakke
Copy link
Contributor

@eibakke eibakke commented Mar 17, 2025

Summary:

We've started using the GTFS validator at Entur in Norway and this message is logged at too high a severity, with no way to disable to logging in the validator itself. If this change is not possible, then it would be great to have a way to opt out of it. Either by defining the validators required when running the validator or by setting a logging config in the validator.

Expected behavior:

Below is a screenshot of what we're seeing currently. We want to change this log message to be at info instead of error severity.

image

We've started using the GTFS validator at Entur and this message is logged at too high a severity, with no way to disable to logging in the validator itself. If this change is not possible, then it would be great to have a way to opt out of it. Either by defining the validators required when running the validator or by setting a logging config in the validator.
Copy link

welcome bot commented Mar 17, 2025

Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:

  • fix: Bug with ssl network connections + Java module permissions.
  • feat: Initial support for multiple @PrimaryKey annotations.
  • docs: update RELEASE.md with new process
    To get this PR to the finish line, please do the following:
  • Read our Contribution Guidelines
  • Follow Google Java style coding standards
  • Include tests when adding/changing behavior
  • Include screenshots

@CLAassistant
Copy link

CLAassistant commented Mar 17, 2025

CLA assistant check
All committers have signed the CLA.

@emmambd emmambd requested a review from davidgamez March 17, 2025 15:35
@@ -230,7 +230,7 @@ public static void printSummary(
}

if (b.length() > 0) {
logger.atSevere().log(b.toString());
logger.atInfo().log(b.toString());
Copy link
Member

Choose a reason for hiding this comment

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

Hoi @eibakke, sorry for the late response. This message is very important as it points out the fact that some notices were not verified due to previous errors like file/column formatting issues. We haven't found a good way to make it to the actual report without confusing non-technical users, as the relationship between validators and notices is unclear. I suggest setting it to warning if this helps to remove excessive logs on your side. Please let me know if this suggestion still is a blocker for your integration.

Suggested change
logger.atInfo().log(b.toString());
logger.atWarning().log(b.toString());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @davidgamez, thank you for following up. Warning sounds like a good middle way. I'll commit the suggestion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi David, I discussed with my colleague Vincent Paturet who pointed out that we would also prefer the ability to tune the logging from the validation runner with our logback.xml. Do you use slf4j?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @eibakke, we use FluentLogger, which uses java.util.logging under the hood. We do have a logging.properties file in the app module that can be extended to incorporate the ValidationRunner class logging levels. However, this is only for the app module. A similar solution can be implemented, depending how you are integrating the tool in your pipeline.
Reference files:

…ationRunner.java

Co-authored-by: David Gamez <1192523+davidgamez@users.noreply.github.com>
@eibakke eibakke requested a review from davidgamez March 25, 2025 07:35
@davidgamez
Copy link
Member

Hi @eibakke, here is a code block that you can use to load a custom logging.properties into your code, link

Copy link
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

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

LGTM!

@davidgamez davidgamez merged commit f1174bd into MobilityData:master Mar 28, 2025
134 checks passed
Copy link

welcome bot commented Mar 28, 2025

🥳 Congrats on getting your first pull request merged!

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.

None yet

3 participants