fix: Make pull query metrics apply only to pull and not also push #6944
Conversation
| * arbitrarily, e.g. /query is used for both push and pull queries so we let the resource | ||
| * determine how to report the metrics. | ||
| */ | ||
| public interface MetricsCallback { |
There was a problem hiding this comment.
nit: since this is used in the old and the new endpoints can we pull it out into its own class? (same for the impl below)
There was a problem hiding this comment.
It wasn't used in the new endpoint when I wrote this. I moved them out and now call it from the new endpoint stream-query as well. I still don't call it from /ws/query because it's a bit harder to do it in these scenarios, though we report latency and basic count from there.
AlanConfluent
force-pushed
the
fix_metrics_only_pull
branch
2 times, most recently
from
9a6ad17 to
84c0aab
Compare
AlanConfluent
force-pushed
the
fix_metrics_only_pull
branch
from
84c0aab to
d939ca6
Compare
There was a problem hiding this comment.
@AlanConfluent for my understanding: which class/line is the actual fix -- it seems to be hidden somewhere with this refactoring change that I'm missing..
| if (throwable == null) { | ||
| pullQueryMetrics.ifPresent(p -> p.recordLatency(startTimeNanos)); | ||
| } | ||
| pullQueryMetrics.ifPresent(p -> p.recordLatency(startTimeNanos)); |
There was a problem hiding this comment.
This seems changing the behavior that even if the result contains an error, we would still record latency. Is it intentional?
Description
This changes pull query metrics to be reported only for pull queries, and not push, since both use the
/queryendpoint.Testing done
Ran tests and manually verified.
Reviewer checklist