Fix #1390: Suppress stack traces for incomplete OSM relations #1431
Conversation
This file contains hidden or 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
When processing regional extracts, incomplete OSM relations (missing
ways or nodes) are expected and common. Previously, these exceptions
were logged with full stack traces, cluttering logs and obscuring
more critical issues.
This change:
- Adds a helper method to detect incomplete relation exceptions
- Logs incomplete relation exceptions as warnings without stack traces
- Keeps full error logging with stack traces for unexpected errors
The fix handles exceptions related to:
- Missing nodes ("Missing location for node")
- Incomplete multipolygons (empty rings, not closed, missing ways)
- GeometryException with stats indicating incomplete relations
Includes a test that verifies incomplete relations are logged as
warnings without stack traces, and fails if the fix is reverted.
Fixes onthegomap#1390
Full logs: https://github.com/onthegomap/planetiler/actions/runs/20720775627 |
- Use pattern matching for instanceof (java:S6201) - Use try-with-resources for TestAppender (java:S2093) - Add comprehensive tests for isIncompleteRelationException method: - Test all message patterns (missing node, multipolygon errors, etc.) - Test GeometryException stat matching - Test exception cause chain recursion - Test edge cases (null, empty message) - Add test for non-incomplete exceptions logged as ERROR This should improve code coverage above the 50% threshold required by SonarCloud.
|
msbarry
reviewed
Jan 15, 2026
Comment on lines
+324
to
+328
| if (isIncompleteRelationException(e)) { | ||
| LOGGER.warn("Incomplete OSM relation {}: {}", relation.id(), e.getMessage()); | ||
| } else { | ||
| LOGGER.error("Error preprocessing OSM relation " + relation.id(), e); | ||
| } |
Contributor
There was a problem hiding this comment.
Thanks for adding this! I already have some similar handling along these lines where these "verbose" exceptions are instances of "GeometryException.Verbose", and GeometryException has a built in geometryException.log("Error processing relation") or geometryException.log("Error processing relation", stats, "osm_relation_error") if we want to include these in the summary of geometry errors that gets printed at the end.
It seems like we might be able to replace this whole PR with just changing these 2 "catch (Exception e)" blocks to something like:
} catch (GeometryException e) {
e.log("Error processing relation " + relation.id());
} catch (Exception e) {
LOGGER.error("Error preprocessing OSM relation {}", relation.id(), e);
}
Contributor
Author
There was a problem hiding this comment.
Frankly, I don't have a strong opinion about the appearance in the log, other than avoiding a scary ERROR and a useless stack trace.
Please feel free to change the PR code in any way you like.
Contributor
There was a problem hiding this comment.
Actually could you share one or a few full stack traces that you're seeing? I tried adding that line but found that GeometryException is actually caught before that point and logged using that method that suppresses verbose logs already.
Contributor
Author
There was a problem hiding this comment.
There is a stack trace in #1390 (comment)
Contributor
There was a problem hiding this comment.
Ahh I see, these geometry exceptions are normally caught and logged quietly (or not logged at all) but your profile asks for the area of a feature, and Unit.java calls applyAndWrapException which propagates any geometry exception that would have been suppressed very loudly.
What do you think the fallback should be when area/length fail to compute? Should it return 0? null? or omit the entire feature
Or more broadly, if any of the properties in a yaml file fail with an exception do you think we should just omit that property? or the entire feature?
Contributor
Author
There was a problem hiding this comment.
There are actually two questions here:
-
Should an incomplete polygon be processed at all? For example, should its attributes be computed?
IMO, if a potential feature does not have a valid geometry it should be dropped as soon as can be. -
How to handle (other) exceptions while computing attribute values?
IMO, it would be best if there is a clear error message with the OSM feature id and type, the layer.feature.name and the attribute name.
That should allow users to fix their definition/code.
Silent drop is generally not a good option. On the other hand, keeping the current stack trace is not very helpful for the user, and an unnecessary support burden for you.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When processing regional extracts, incomplete OSM relations (missing ways or nodes) are expected and common. Previously, these exceptions were logged with full stack traces, cluttering logs and obscuring more critical issues.
This change:
The fix handles exceptions related to:
Includes a test that verifies incomplete relations are logged as warnings without stack traces, and fails if the fix is reverted.
Fixes #1390