client-api: Optionally include the current program hash in the database info - #5678
client-api: Optionally include the current program hash in the database info#5678kim wants to merge 1 commit into
Conversation
…se info Useful in certain circumstances, including observability. Including the information in the info endpoint is the obvious choice without introducing further machinery. It comes with some caveats, however: - If the database is not running, retrieving the module hash as implemented will start it. This may not be desirable, which is why the behavior is opt-in via the `current-hash` query parameter. - If the database is suspended, paused, or faulty, determining the hash is not possible. For the intended purpose, this is sufficient, as absence is an error condition.
gefjon
left a comment
gefjon
left a comment
There was a problem hiding this comment.
Why is the response encoded as JSON via SATS?
I think this is a historical artifact, not any particular reason.
Do we have documented consumers of the endpoint? The CLI doesn't seem to talk
to it.
Not to my knowledge. In my opinion, if we don't have tests of it and the CLI doesn't touch it, then it's not stable.
[extending the database info route] is the obvious choice without introducing further machinery.
I don't think I agree with this. It seems like it's actually pretty non-obvious to extend the existing route without breaking things, and we end up with a more complicated API and worse error handling than if we just added a new endpoint.
| let current_program = async { | ||
| current_hash.then_some(())?; | ||
| let host = worker_ctx.leader(database.id).await.ok()?; | ||
| let module = host.module().await.ok()?; | ||
| Some(module.info().module_hash) | ||
| } | ||
| .await; |
There was a problem hiding this comment.
I don't love swallowing failures here; it seems like the kind of thing that's going to confuse and frustrate someone 6 months or a year from now.
|
If you think a new, undocumented, mostly internally-used endpoint is preferable, please specify said endpoint. |
2 participants
Useful in certain circumstances, including observability.
Including the information in the info endpoint is the obvious choice
without introducing further machinery. It comes with some caveats,
however:
If the database is not running, retrieving the module hash as
implemented will start it.
This may not be desirable, which is why the behavior is opt-in via the
current-hashquery parameter.If the database is suspended, paused, or faulty, determining the hash
is not possible.
For the intended purpose, this is sufficient, as absence is an error
condition.
Expected complexity level and risk
2
Testing
The feature is (implicitly) tested elsewhere.
I am, however, wondering:
to it.
Given answers to those questions, it would be worthwhile to add a smoketest that
fails if the shape of the response changes.