count persisted cache hits

[Geal]

Fix #1014

This counts properly persisted_cache_hits, but not persisted_cache_misses. Apparently, in that case we do not even get into the update_apollo_metrics function, so I suspect the error returned by APQ skips the reporting phase. I also do not know yet under which stats_report_key I should record APQ cache misses, because if we get a cache miss, we do not know which query it was linked to.

Looking at this code it's not clear to me which stats report key I should use: https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-core/src/plugin/usageReporting/plugin.ts#L644-L682

[netlify]

✅ Deploy Preview for apollo-router-docs ready!

Name	Link
🔨 Latest commit	a40abdd
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/628b6ab63402a900083b7d7b
😎 Deploy Preview	https://deploy-preview-1117--apollo-router-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[Geal]

@glasser could you provide some context on how persisted_query_misses should be handled? If I understand correctly, in case of APQ cache miss, we cannot know the expected query, so there's no associated stats report key?

In the protobuf schema, I also see the Trace message has these fields:

  // Was this query specified successfully as a persisted query hash?
  bool persisted_query_hit = 21;
  // Did this query contain both a full query string and a persisted query hash?
  // (This typically means that a previous request was rejected as an unknown
  // persisted query.)
  bool persisted_query_register = 22;

Is it counted separately from the QueryLatencyStats message?

[glasser]

@Geal persisted_query_register in Trace matches persisted_query_misses in aggregated stats and is a better name for the same thing. Perhaps we should rename the aggregated field to match the trace field.

Looks from monorepo log -S persisted_query_register etc that the history was:

• When we first implemented APQs (in engineproxy!) we added stats about it with the persisted_query_misses misnomer, and we didn't actually mention persisted query-ness in traces at all. Back then Traces and Stats were sent to our servers completely separately and there was no concept of taking a trace and aggregating it to stats.
• When we implemented the reporting protocol for "direct reporting from Apollo Server" it initially was implemented as "only send traces, never send stats, all aggregation happens inside Apollo's servers". (This was intended as a short-term plan to last a month or two before we went back to reporting-agent-style aggregation and accidentally lasted a couple years oops.) So now we had the concept of turning traces into stats, which meant all data that go into stats had to be represented in traces. So I added a relevant field in https://github.com/mdg-private/monorepo/pull/940 and named it persisted_query_register instead of persisted_query_miss because it's more accurate. And wrote the comment you quoted above. However I never went back and renamed the stats field.

So yeah, don't actually count "misses", count "registers" since those typically come right after a miss.

Does that make sense?

Note that Apollo Server doesn't have a way of reporting persisted query not-found or not-supported errors (or like "no query provided"); it has a comment suggesting that that might be a good idea, but for now they just aren't reported at all.

[glasser]

(FWIW, while implementing this is a good idea for feature-parity's sake, I don't believe Studio actually shows this data anywhere. In fact I don't think it writes the aggregated data anywhere and it probably does save the Trace flags along with the Trace but doesn't expose it via the API, let alone show it in the UI.)

[Geal]

ok, so then I'll record query registers in persisted_cache_misses, thank you for the explanation!

[BrynCooke]

LGTM. Needs a changelog entry.