> 1. Is there any reason you wouldn't want to prefer the whole RequestMessage > over just the request id as the first argument to all of those methods?
> 2. For onQueryError() I'd wonder if it isn't better to use the actual > Throwable rather than just the error message Those are good ideas, I've made the change in https://github.com/apache/tinkerpop/compare/3.6-dev...tkolanko:tinkerpop:3.6-dev?expand=1 > 3. For onQueryTimeout() does this get called in addition to onQueryError() > - I suppose it doesn't? I sense you're mapping to the GremlinExecutor > lifecycle, but I wonder if that's right? Maybe for a Graph Providers' > purposes it's better to get a single call for an error (where a timeout is > a form of an error) and the GraphManager implementation figures out what to > do with it? The LifeCycle for the Gremlin Executor does not include a throwable for query timeouts ( https://github.com/apache/tinkerpop/blob/3a5917be789612acb31a7043f47f67c6e16b31f6/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java#L480). In https://github.com/apache/tinkerpop/compare/3.6-dev...tkolanko:tinkerpop:3.6-dev?expand=1 I've made a change to create a new TimeoutException and pass that into the onQueryError method and removed the onQueryTimeout method On 2022/10/06 13:07:50 Stephen Mallette wrote: > Generally speaking I think this is a nice idea. A few questions based on > your prototype: > > 1. Is there any reason you wouldn't want to prefer the whole RequestMessage > over just the request id as the first argument to all of those methods? > 2. For onQueryError() I'd wonder if it isn't better to use the actual > Throwable rather than just the error message > 3. For onQueryTimeout() does this get called in addition to onQueryError() > - I suppose it doesn't? I sense you're mapping to the GremlinExecutor > lifecycle, but I wonder if that's right? Maybe for a Graph Providers' > purposes it's better to get a single call for an error (where a timeout is > a form of an error) and the GraphManager implementation figures out what to > do with it? > > Overall, what you have seems like a good start and proof that this approach > holds water. Thanks! > > > > On Tue, Oct 4, 2022 at 1:54 PM Thomas Kolanko <[email protected]> wrote: > > > I created https://issues.apache.org/jira/browse/TINKERPOP-2806 to enhance > > the GraphManager interface to allow providers to opt into > > receiving notifications when scripts/queries are processed. > > > > Ideally what we would like as a provider is to be able to tell when a query > > starts, completes, errors or encounters a timeout so we can provide metrics > > showing success / error rates by graph. Currently as a provider the > > gremlin-server is a black box where you have no insight to what is being > > processed. TINKERPOP-2806 proposes adding some default methods to the > > GraphManager interface which will allow providers to opt into handling the > > lifecycle of script/query execution. I created a prototype of what this > > could look like at > > > > https://github.com/apache/tinkerpop/compare/3.6-dev...tkolanko:tinkerpop:3.6-dev?expand=1 > > > > Inside AbstractEvalOpProcessor and TraversalOpProcessor I am getting the > > GraphManager from context and calling the appropriate method depending > > where in the lifecycle the script/query processing is > > >
