adds ADR to expose tuning parameters - #978
Conversation
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
I support option B. I think strict typing is critical here. As I understand it, option C would see any JSON object and validate it based on data type alone. Even if it's probably fine in practice, delegating validation elsewhere in the codebase feels risky to me, and not worth the perceived benefits.
|
|
||
| This creates a mismatch for downstream clients that validate against introspection-derived schema artifacts, including graphql-client. Those clients fail validation because the schema they consume does not include arguments that our internal callers (timdex-ui in particular) need to send. | ||
|
|
||
| Our GraphQL API is public, but exposing advanced tuning arguments is acceptable, even if most external users do not need them. We can state clearly they are likely not useful to most users in the documentation exposed by the Introspection queries. |
There was a problem hiding this comment.
I think it would be useful to state clearly why it's acceptable to expose advanced tuning arguments. I assume the worst case scenario is that users who fiddle with the settings might get weird results, which feels low risk.
There was a problem hiding this comment.
Great point. Yes, the risk for all of our current tuning parameters is excluding results that would have been useful or returning way more irrelevant results after the main good results. I'll update to clarify explicitly why this is low risk as you suggested.
There was a problem hiding this comment.
Updated with wording clarifying why it is okay to expose our tuning parameters.
|
|
||
| Initial recognized keys: | ||
|
|
||
| - mustBoostThreshold (Float, 0.0 to 1.0) |
There was a problem hiding this comment.
Probably not worth mentioning in this doc, as it's an implementation detail, but I don't think we've done this kind of input limitation in TIMDEX before. I'd imagine it would be pretty straightforward.
|
|
||
| Tips: | ||
|
|
||
| - Add test coverage for accepted and invalid tuning values. |
There was a problem hiding this comment.
Ignore my comment above about input validation. I think this addresses it, since test coverage would imply existing validation code.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
I support option B, for all the stated reasons in the document and the comments here. Having these included in the public schema should prove vastly more workable when it comes to actually using these parameters during measurement and tuning operations.
(I went down a bit of a rabbit hole trying to imagine how these different options might work from the Quepid interface before realizing that the UI application will be the thing mediating between the querystring which Quepid exposes and the GraphQL syntax that is envisioned here - as evidenced by how we already handle this in a GeoData context for the GeoBox and GeoDistance constructs.)
d103d51 to
229f9bf
Compare
4 participants
No description provided.