opentripplanner / OpenTripPlanner

An open source multi-modal trip planner
http://www.opentripplanner.org
Other
2.18k stars 1.02k forks source link

Limit result size and execution time in TransModel GraphQL API #5883

Closed vpaturet closed 3 months ago

vpaturet commented 4 months ago

Summary

This PR builds upon #5881 and provides a GraphQL instrumentation that limits both the result size and the execution time of a GraphQL query.

This instrumentation is meant to replace both the OTPRequestTimeoutInstrumentation introduced in #5881 and the MaxQueryComplexityInstrumentation.

Note: MaxQueryComplexityInstrumentation does not check the runtime complexity of a query execution. It performs only a static analysis on the query text. By default it checks only the number of fields in the query text, which has limited value.

Note: the default max size is intentionally high and might be subsequently tuned in follow-up PRs.

Note: in the TransModel API, resolvers are run sequentially in the calling thread (that is: the web server thread). Thus the instrumentation object that is shared by all resolvers is accessed by only one thread and the use of an AtomicLong is actually not required. However if this instrumentation were to be ported to another API that uses multi-threaded resolving (GTFS API?), the code should work out-of-the-box and concurrently abort the resolvers (see org.opentripplanner.apis.transmodel.support.AbortOnTimeoutExecutionStrategy) This is however not tested and not in the scope of this PR.

Issue

No.

Unit tests

No.

Documentation

No.

vpaturet commented 4 months ago

@t2gran @leonardehrenfried I intend to use a router-config parameter instead of a header for parameterizing the result size. Do you know the reason why this is currently exposed as a header?

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 28.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 68.61%. Comparing base (ba67f17) to head (8e5873a). Report is 161 commits behind head on dev-2.x.

Files Patch % Lines
...s/transmodel/MaxFieldsInResultInstrumentation.java 0.00% 14 Missing :warning:
...entripplanner/apis/transmodel/TransmodelGraph.java 0.00% 2 Missing :warning:
...opentripplanner/apis/transmodel/TransmodelAPI.java 0.00% 1 Missing :warning:
...standalone/config/sandbox/TransmodelAPIConfig.java 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev-2.x #5883 +/- ## ============================================= + Coverage 68.48% 68.61% +0.12% - Complexity 16703 16823 +120 ============================================= Files 1916 1927 +11 Lines 72660 72942 +282 Branches 7448 7476 +28 ============================================= + Hits 49762 50047 +285 + Misses 20333 20321 -12 - Partials 2565 2574 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vpaturet commented 4 months ago

@t2gran @leonardehrenfried I intend to use a router-config parameter instead of a header for parameterizing the result size. Do you know the reason why this is currently exposed as a header?

Discussed during the developer meeting: the intent of this header was to let a proxy/bff relax the limit on the GraphQL complexity. This proxy/bff is not in use anymore, thus the header can be safely removed.

t2gran commented 3 months ago

What should we do about the "since" field on the config? Should we remove it or enforce it?

vpaturet commented 3 months ago

I added anyway the version that was missing on the new parameter.

leonardehrenfried commented 3 months ago

What should we do about the "since" field on the config? Should we remove it or enforce it?

I think the current model where it's optional works quite well. I think having this information is often useful.

t2gran commented 3 months ago

Reply: We can discuss it on the next developer meeting. The goal was to get rid of the NA at some point. But, omitting the since tag have the same effect - except it is not visible any more. Having it as optional case people to forget it.