Skip to content

Catch interpolation exceptions (OTP2) - #3412

Merged
t2gran merged 9 commits into
opentripplanner:dev-2.xfrom
entur:otp2_interpolation_exceptions
Sep 9, 2021
Merged

t2gran merged 9 commits into
opentripplanner:dev-2.xfrom
entur:otp2_interpolation_exceptions

Conversation

@gmellemstrand

Copy link
Copy Markdown
Contributor

Summary

This is an OTP2 rebase of #3078 with no changes.

Changelog

Changelog entry updated

@gmellemstrand
gmellemstrand requested a review from a team April 9, 2021 13:16
evansiroky
evansiroky previously approved these changes Apr 13, 2021
@gmellemstrand

Copy link
Copy Markdown
Contributor Author

We agreed in today's OTP meeting to increase the log levels to warn, but accept the PR.

@evansiroky

Copy link
Copy Markdown
Contributor

I believe that I set the log levels to debug since there could be thousands of these in certain cases where not enough elevation tiles are available for the full extent of the street network. The logs will be a lot noisier with this change.

@landonreed

Copy link
Copy Markdown
Member

Maybe a good compromise would be to count the exceptions and log a single message (with this count) at the end of processing (potentially with some other helpful identifying information like a bounding box)?

@t2gran t2gran added this to the 2.1 milestone Apr 14, 2021
@t2gran

t2gran commented Apr 15, 2021

Copy link
Copy Markdown
Member

I agree will @landonreed, I would implement this with a LogThrottle class like this:

// Example use
LoggerThrottle logThrottle = new LoggerThrottle();
:
logThrottle.throttle("Bla bla bal, edge {}", msg -> log.warn(msg, edge));

// Trottle class
package org.opentripplanner.util;

public class LoggerThrottle  {
    private final Map<String, Integer> msgCounts = new HashMap<>();

    public void throttle(String message, Consumer<String> logOp) {
        Integer counter = msgCounts.get(message);
        if(counter == null) {
            msgCounts.put(message, 1);
            logOp.accept(message);
        }
        else {
            msgCounts.put(message, counter+1);
        }
    }

    public List<String> summary() {
        var list = new ArrayList<String>();
        for (Entry<String, Integer> e : msgCounts.entrySet()) {
            list.add(String.format("LOG THROTTLE - %d events: %s", e.getValue(), e.getKey()));
        }
        return list;
    }
}

One little remark: do not use logThrottle.throttle("...", log::warn);, this will cause the log event to happen inside the lambda and the link to the code will be invalid (We want be able to navigate to the code line).

@t2gran

t2gran commented Apr 15, 2021 •

Copy link
Copy Markdown
Member

LogThrottling is useful in many places in the graph builder process - hence the trouble to make a LoggerThrottle class. We do not care about the details missed. The data details should be logged to the issue report instead. For this particular case, we can wait until someone request it to be logged in the data issue report.

@gmellemstrand
gmellemstrand requested a review from a team as a code owner September 2, 2021 00:18
@t2gran
t2gran merged commit c1b5f10 into opentripplanner:dev-2.x Sep 9, 2021
@t2gran
t2gran deleted the otp2_interpolation_exceptions branch September 9, 2021 10:21
@leonardehrenfried leonardehrenfried mentioned this pull request Dec 15, 2022
4 of 8 tasks
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.

6 participants