-
Notifications
You must be signed in to change notification settings - Fork 45
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
core: aggregate errors that happen too often during infra loading #10345
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10345 +/- ##
==========================================
- Coverage 81.64% 81.63% -0.01%
==========================================
Files 1066 1066
Lines 105735 105735
Branches 727 727
==========================================
- Hits 86328 86321 -7
- Misses 19366 19373 +7
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8134a05
to
3e2fbb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution is quite elegant.
But how about just raising the log level when deployed (or even default to info)?
Then we can also consider lowering log-level for electrification (and move this to a more important tracking on the data-production side).
If we are able to target specific log-levels on specific areas of the code (like warning for infra import and debug for the rest), it may also avoid revision of all log-levels of the component.
core/kt-osrd-utils/src/main/kotlin/fr/sncf/osrd/utils/LogAggregator.kt
Outdated
Show resolved
Hide resolved
core/kt-osrd-signaling/src/main/kotlin/fr/sncf/osrd/signaling/impl/BlockBuilder.kt
Outdated
Show resolved
Hide resolved
30f997a
to
103acf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to merge IMO.
The previous comment about adjusting log-level is still "alive" IMO, but any pick is OK.
We should probably look into this at some point (refining the log levels and what we want in prod). But I don't think it would be enough, some errors are important enough to be logged in prod environments, without necessarily having all the details. Like I've just added an aggregation for this error that happens in prod but (usually) not in local, it's quite important to know that it's there but there's no need to log it dozens of times per request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, GJ!
Signed-off-by: Eloi Charpentier <[email protected]>
ba916ae
to
666849a
Compare
Fix #8773
After:
Before: same thing but over 2k2 lines