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.

Sign up for GitHub

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 freeform guardrails #918

Merged
merged 6 commits into from
Dec 31, 2023
Merged

Fix freeform guardrails #918

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b6c767
Fix freeform guardrails
ansoncfit Dec 19, 2023
2481a93
Add final modifier to PathResult constants
ansoncfit Dec 26, 2023
06e479f
Merge branch 'dev' into freeform-guardrails
abyrd Dec 28, 2023
9690d9d
Use specific AnalysisServerException
ansoncfit Dec 29, 2023
9abf498
Filter stack traces sent to UI
ansoncfit Dec 29, 2023
6a6ab47
build filtered stack trace directly from throwable
abyrd Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions src/main/java/com/conveyal/analysis/results/MultiOriginAssembler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import com.conveyal.file.FileStorage;
import com.conveyal.file.FileStorageFormat;
import com.conveyal.r5.analyst.PointSet;
import com.conveyal.r5.analyst.cluster.PathResult;
import com.conveyal.r5.analyst.cluster.RegionalTask;
import com.conveyal.r5.analyst.cluster.RegionalWorkResult;
import com.conveyal.r5.util.ExceptionUtils;
import org.slf4j.Logger;
Expand Down Expand Up @@ -89,21 +91,27 @@ public MultiOriginAssembler (RegionalAnalysis regionalAnalysis, Job job, FileSto
this.job = job;
this.nOriginsTotal = job.nTasksTotal;
this.originsReceived = new BitSet(job.nTasksTotal);
// Check that origin and destination sets are not too big for generating CSV files.
if (!job.templateTask.makeTauiSite &&
job.templateTask.destinationPointSetKeys[0].endsWith(FileStorageFormat.FREEFORM.extension)
) {
// This requires us to have already loaded this destination pointset instance into the transient field.
PointSet destinationPointSet = job.templateTask.destinationPointSets[0];
if ((job.templateTask.recordTimes || job.templateTask.includePathResults) && !job.templateTask.oneToOne) {
if (nOriginsTotal * destinationPointSet.featureCount() > MAX_FREEFORM_OD_PAIRS ||
destinationPointSet.featureCount() > MAX_FREEFORM_DESTINATIONS
) {
throw new AnalysisServerException(String.format(
"Freeform requests limited to %d destinations and %d origin-destination pairs.",
MAX_FREEFORM_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
));
}
// If results have been requested for freeform origins, check that the origin and
// destination pointsets are not too big for generating CSV files.
RegionalTask task = job.templateTask;
if (!task.makeTauiSite && task.destinationPointSetKeys[0].endsWith(FileStorageFormat.FREEFORM.extension)) {
// This requires us to have already loaded this destination pointset instance into the transient field.
PointSet destinationPointSet = task.destinationPointSets[0];
int nDestinations = destinationPointSet.featureCount();
int nODPairs = task.oneToOne ? nOriginsTotal : nOriginsTotal * nDestinations;
if (task.recordTimes &&
(nDestinations > MAX_FREEFORM_DESTINATIONS || nODPairs > MAX_FREEFORM_OD_PAIRS)) {
throw new AnalysisServerException(String.format(
Copy link
Member Author

@ansoncfit ansoncfit Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is throw AnalysisServerException.badRequest(... preferable here?

Is it straightforward to apply filterStackTrace (moved to ExceptionUtils in #921) to the message eventually passed along to the UI?

Copy link
Member

@abyrd abyrd Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On your second question: yes, I think so. It looks like we could apply filterStackTrace to the message parameter in each call to HttpApi#respondToException (there are five of them just above the method definition).

Copy link
Member

Choose a reason for hiding this comment

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

On your first question: it looks like calling badRequest would yield an identical AnalysisServerException instance but with type BAD_REQUEST instead of UNKNOWN. It looks like the only effect would be the string BAD_REQUEST instead of UNKNOWN in a field in the JSON returned to the UI. Assuming this does not change UI behavior in any problematic way, yes badRequest does seem to better represent the source of the exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out that method. I think we want to apply it to the stackTrace, not the message. I committed that below for testing on staging, but feel free to revert.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right. It was late when I was reading that and I mistakenly thought the callers were passing e.getStackTrace() as the message. This was probably driven by some kind of wishful thinking because I realized filterStackTrace is so narrowly suited to output from one method (throwable.printStackTrace via ExceptionUtils.stackTraceString) that it probably shouldn't be public and applicable to the more general String type. This is a minor detail but it's located in error handling code that should be foolproof (things could rapidly get annoying to debug if the signal path for the debugging messages themselves is failing). It looks like it is failsafe and would at worst return "Unknown stack frame" but there could be some edge case we're not thinking of.
Of course in current usage, both places the method is used are filterStackTrace(errorEvent.stackTrace) so we know it's always being applied to a final field that's the output of ExceptionUtils.filterStackTrace(). This should work fine but I'm still thinking about ways to clean up the method signature.
Option A: have the public method be public static String filterStackTrace (Throwable throwable) then call that in the ErrorEvent constructor. This would cause all console log traces to be filtered. All others (Slack) are already filtered.
Option B: move filterStackTrace back to a private method on ErrorEvent and always use it via a public wrapper method that only reads from ErrorEvent.stackTrace. (Won't work because we need it in RegionalWorkResult)
Option C: Add a field to ErrorEvent so it has both the full and filtered stack trace
Option D: keep the entire Throwable in a private field on the ErrorEvent and generate the stack traces on demand

Copy link
Member

@abyrd abyrd Dec 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I did option C as it ensures the trace string always came from the right method but still allows for verbose stack traces in console logs. Seems to work in local testing on some missing regional results.

D looks cleaner, but it defers processing of the stack trace to the moment the log or slack message is written, thus deferring some potential failures to those background event handling threads. I'd rather front-load the stack trace processing to the point where the ErrorEvent is created, containing any delays or problems closer to the source of the original error.

"Travel time results limited to %d destinations and %d origin-destination pairs.",
MAX_FREEFORM_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
));
}
if (task.includePathResults &&
(nDestinations > PathResult.MAX_PATH_DESTINATIONS || nODPairs > MAX_FREEFORM_OD_PAIRS)) {
throw new AnalysisServerException(String.format(
"Path results limited to %d destinations and %d origin-destination pairs.",
PathResult.MAX_PATH_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
));
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public class PathResult {
* These results are returned to the backend over an HTTP API so we don't want to risk making them too huge.
* This could be set to a higher number in cases where you know the result return channel can handle the size.
*/
public static int maxDestinations = 5000;
public static final int MAX_PATH_DESTINATIONS = 5_000;

private final int nDestinations;
/**
Expand All @@ -49,7 +49,7 @@ public class PathResult {
public final Multimap<RouteSequence, Iteration>[] iterationsForPathTemplates;
private final TransitLayer transitLayer;

public static String[] DATA_COLUMNS = new String[]{
public static final String[] DATA_COLUMNS = new String[]{
"routes",
"boardStops",
"alightStops",
Expand All @@ -70,8 +70,8 @@ public PathResult(AnalysisWorkerTask task, TransitLayer transitLayer) {
// In regional analyses, return paths to all destinations
nDestinations = task.nTargetsPerOrigin();
// This limitation reflects the initial design, for use with freeform pointset destinations
if (nDestinations > maxDestinations) {
throw new UnsupportedOperationException("Number of detailed path destinations exceeds limit of " + maxDestinations);
if (nDestinations > MAX_PATH_DESTINATIONS) {
throw new UnsupportedOperationException("Number of detailed path destinations exceeds limit of " + MAX_PATH_DESTINATIONS);
}
}
iterationsForPathTemplates = new Multimap[nDestinations];
Expand Down