yabeda-graphql icon indicating copy to clipboard operation
yabeda-graphql copied to clipboard

Measure overall query execution times by operation name

Open skateman opened this issue 2 years ago • 2 comments

The implementation is probably organized in a bad way, so if I get some feedback, I will adjust it as you please. The overall idea is to have a runtime histogram for queries by operation name. Of course this only works if the query name is set, but the point here is to see which are the slow queries.

skateman avatar Oct 25 '22 08:10 skateman

Hey, thanks for your contribution!

I have one concern and one question:

  1. Concern: as operation name is set by clients, tracking it in labels opens a possibility to attacks on monitoring: malicious (or just reckless) client can send random operation name with every request, leading to creation of large number of time series, overloading monitoring software (or resulting in a huge bills from cloud one). See this caution on labels in Prometheus docs for example, but it applies to all monitoring systems.

    Maybe make it opt-in? Yabeda uses anyway_config, so it should be pretty easy to add a configuration.

  2. Are you sure that your code collects duration of the whole operation with all subqueries in it?

    I don't remember all GraphQL-Ruby stuff well. but while after_query instrumentation method seems to be executed after whole query execution, cache inside it contains data per every field, so if operation consists of more than one query field, data will be reported inaccurately (I'm not sure, but that is my fear).

    Maybe you need to sum durations of operation queries first.

Envek avatar Oct 25 '22 12:10 Envek

Sorry for the late reaction, and thanks for the quick reply :pray:

  1. Would it be reasonable to have a regex on the operation name to limit the attack surface? It could be also configured via anyway, but a default one that is safe for prometheus would not hurt. Also the opt-in of the feature itself can be done as you request on top of this.
  2. I will look into this one more thoroughly...

skateman avatar Nov 03 '22 08:11 skateman