Update Apollo usage reporting to include persisted query metrics #7166
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 766372c9bdbcceb184faa00d |
This comment has been minimized.
This comment has been minimized.
|
CI performance tests
|
bonnici
force-pushed
the
njm/P-416/pq-reporting
branch
from
91e1978 to
bf3cfcb
Compare
bonnici
force-pushed
the
njm/P-416/pq-reporting
branch
from
c3081a3 to
dd86724
Compare
bonnici
changed the title
bonnici
force-pushed
the
njm/P-416/pq-reporting
branch
from
c67f5ad to
a96f8e9
Compare
| pub(crate) fn with_pq_id(&self, pq_id: Option<String>) -> UsageReporting { | ||
| let mut updated_usage_report = self.clone(); | ||
| updated_usage_report.pq_id = pq_id; | ||
| updated_usage_report | ||
| } |
There was a problem hiding this comment.
I think this would be a bit cleaner with struct update syntax:
pub(crate) fn with_pq_id(&self, pq_id: Option<String>) -> UsageReporting {
UsageReporting {
pq_id,
..self.clone()
}
}| pub(crate) struct UsageReporting { | ||
| /// The `stats_report_key` is a unique identifier derived from schema and query. | ||
| /// Metric data sent to Studio must be aggregated | ||
| /// via grouped key of (`client_name`, `client_version`, `stats_report_key`). | ||
| pub(crate) stats_report_key: String, | ||
| /// The operation name, or None if there is no operation name | ||
| pub(crate) operation_name: Option<String>, | ||
| /// The normalized operation signature, or None if there is no valid signature | ||
| pub(crate) operation_signature: Option<String>, | ||
| /// The error key to use for the stats report, or None if there is no error | ||
| pub(crate) error_key: Option<String>, | ||
| /// The persisted query ID used to request this operation, or None if the query was not requested via PQ ID | ||
| pub(crate) pq_id: Option<String>, |
There was a problem hiding this comment.
Would it be crazy to split this into an enum? I feel like we're already splitting logic a lot here in the for_error case.
enum UsageReporting {
ERROR{key: String},
OPERATION{name: String, signature: String, pq_id: Option<String>}
}There was a problem hiding this comment.
I started going down this route but it makes code a bit too complicated. Also there is a possible future where we'll want to be able to have an error that still has referenced fields by type, or maybe an operation name/signature (via query metadata). We'd need to update the reporting code, but if we do that we'd definitely need to change back from an enum.
There was a problem hiding this comment.
Gotcha. I'm surprised the code ends up more complex that way. Every time I saw a for_error() in your code I was thinking "that'd look nice as a UsageReporting::Error() instead". And I'd imagine the internal logic is basically just doing match instead of the if/else on what attrs exist in the current code.
Understood on the possible future state -- In my head it'd be easy to add those init params if we need them to the error enum, but I haven't worked with enums in practice so I'll trust your experience on all this 😄
There was a problem hiding this comment.
I had another go and I think I figured it out
| pub(crate) fn with_pq_id(&self, maybe_pq_id: Option<String>) -> UsageReporting { | ||
| match self { | ||
| UsageReporting::Operation(op_details) | ||
| | UsageReporting::PersistedQuery { | ||
| operation_details: op_details, | ||
| .. | ||
| } => match maybe_pq_id { | ||
| Some(pq_id) => UsageReporting::PersistedQuery { | ||
| operation_details: op_details.clone(), | ||
| persisted_query_id: pq_id, | ||
| }, | ||
| None => UsageReporting::Operation(op_details.clone()), | ||
| }, | ||
| // PQ ID has no effect on errors | ||
| UsageReporting::Error { .. } => self.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
This fn feels a little weird to me that we're only maybe making a pq report. It would be a bit cleaner if we forced the caller to say if it's a pq or not (make pq_id non-optional), but that'd still be weird in the error case. Not sure of a better solution and I'm not super opinionated about it 🤷
There was a problem hiding this comment.
I've updated it so PQ ID is required, which makes it a bit nicer.
bonnici
force-pushed
the
njm/P-416/pq-reporting
branch
from
f8b67aa to
93702d5
Compare
Review by commit is recommended.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩