feat: return runtime versions used by the application with a doctor hook #81

zimeg · 2024-03-21T00:04:06Z

Summary

This PR introduces a doctor hook that returns runtime versions for the application's execution environment in a structured way. The tests demonstrate the expected response shape!

Information about the version of Deno used for hosted apps is collected from: https://api.slack.com/slackcli/metadata.json

Testing

In your own sample application and with a prepared version of the CLI, update your slack.json to include the following then run slack doctor:

{
  "hooks": {
    "get-hooks": "deno run -q --allow-read --allow-net https://deno.land/x/[email protected]/mod.ts",
    "doctor": "deno run --allow-read --allow-net https://raw.githubusercontent.com/slackapi/deno-slack-hooks/a8f80e7c4870dd5aa35612872dc6ca24908a7d40/src/doctor.ts"
  }
}

Note: You can replace the SHA with the latest commit! But this should work pretty well.

Notes

~~Comparing the current version with the minimum is a task left to the CLI! However this might be better handled by a hook? I'm really not sure...~~ 🤔

Update: It has been decided that this is the better way. So now it's been changed.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2024-03-21T00:05:16Z

Codecov Report

Attention: Patch coverage is 93.50649% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 78.53%. Comparing base (5469b14) to head (b81f76f).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/doctor.ts	91.80%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   77.24%   78.53%   +1.29%     
==========================================
  Files          16       17       +1     
  Lines         813      876      +63     
  Branches      111      129      +18     
==========================================
+ Hits          628      688      +60     
- Misses        182      187       +5     
+ Partials        3        1       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

WilliamBergamin

I don't have much to say about the code 💯

Comparing the current version with the minimum is a task left to the CLI! However this might be better handled by a hook? I'm really not sure... 🤔

It would make sense to have the hook compare the current versions I feel like the version patterns might be slightly difference from runtime to runtime, this is also how the check update handles it

WilliamBergamin · 2024-03-21T13:09:03Z

src/doctor.ts

+    const message = Deno.version.deno !== version
+      ? `Applications deployed to Slack use Deno version ${version}`
+      : undefined;
+    return { minimum: version, message };


🤔 I'm not sure if the deno-runtime used by run on slack infra is the "minimum" version I think it might just be the version being used since we did not implement a way for developers to set a specific version they can't change it

For example if a developer is using new features found in Deno 1.41.x and run on slack support 1.37.x that does not have those features then the developers code would fail so 1.37.x is not the minimum version

Yeah we discussed this point in the spec @zimeg wrote. The minimum is just the payload shape, but you can see in the messaging to the user we don't actually mention minimum. This is intentional. We are very carefully choosing our words when raising this to the user so that we don't imply a 'minimum' version but rather communicate that ROSI uses one EXACT version.

I agree that "minimum" isn't a super ideal word for this case but it gives us flexibility across other SDKs to specify certain runtime requirements IMO.

With the current and minimum version comparison moving into the hooks I'm super interested in how this output might change. Or maybe it doesn't? But wanted to share an example for reference:

Without special error handling in the CLI, the hook can output this messaging which I think is pretty alright! All of this info is coming from the hook.

Just to circle back on this discussion with the latest updates- minimum no longer exists. Neither does latest. At least in the hook response. Only message and errors.message can provide additional detail.

LGTM also 👍
The hook can also use the protocol to share an error message correct?

src/doctor.ts

filmaj

Tested this with the CLI PR you have up as well as with an old (2.5.0) version of the CLI - works well in both cases!

Left a few suggestions but looking real good and close to shipping! Nicely done.

Comparing the current version with the minimum is a task left to the CLI! However this might be better handled by a hook? I'm really not sure... 🤔

Good point. I suppose in order to try to adhere to the design of 'the CLI is dumb' we should err on the side of pushing complexity to the SDKs. We should probably make a decision on this at this point, while we are baking this new hook, rather than delay on this down the road, as that would complicate rollout in the future (would need to first roll out hook changes, possibly consider backwards compatibility, and so on).

My vote is to push the comparison logic to the SDK, which as Will points out is what we do for check-update too.

src/doctor.ts

filmaj · 2024-03-21T15:27:48Z

src/doctor.ts

+    const metadataURL = "https://api.slack.com/slackcli/metadata.json";
+    const response = await fetch(metadataURL);
+    if (!response.ok) {
+      throw new Error("Failed to collect upstream CLI metadata");


Let's add the error details here when we raise this, at least the HTTP response code.

Included the status code here to hint at the error, but let me know if you think more would be better!

In the same vein as my last comment, perhaps we pump the error details over the CLI/SDK protocol's diagnostic line to allow the CLI to more gracefully handle showing the error in the right place (high level error in the output, with a pointer to the debug log, where the full details can be shown)? WDYT?

filmaj · 2024-03-21T15:28:24Z

src/doctor.ts

+    const metadata = await response.json();
+    const version = metadata?.["deno-runtime"]?.releases[0]?.version;
+    if (!version) {
+      throw new Error("Failed to find the minimum Deno version");


Let's add the metadata.cli response payload as part of the error too.

Good call. I think this is useful, even if no obvious remediation exists

Hmm, thoughts on maybe pumping the specific error to the debug log and not messing up the delicate doctor template? We may have to use the protocol's differing respond vs. log methods to enable the CLI to distinguish between the two.

filmaj · 2024-03-21T15:29:53Z

src/doctor.ts

+    const message = Deno.version.deno !== version
+      ? `Applications deployed to Slack use Deno version ${version}`
+      : undefined;
+    return { minimum: version, message };


Yeah we discussed this point in the spec @zimeg wrote. The minimum is just the payload shape, but you can see in the messaging to the user we don't actually mention minimum. This is intentional. We are very carefully choosing our words when raising this to the user so that we don't imply a 'minimum' version but rather communicate that ROSI uses one EXACT version.

filmaj · 2024-03-21T15:31:25Z

src/doctor.ts

+    return { minimum: version, message };
+  } catch (err) {
+    if (err instanceof Error) {
+      return { error: { message: err.message } };


To my earlier comments, can we check to make sure that the HTTP response code/body are at a minimum raised if the error caught is one of the HTTP or payload-parsing code fails? Perhaps add that as part of the test suite (if it's not already there).

zimeg · 2024-03-21T17:47:30Z

@WilliamBergamin @filmaj appreciate all of this feedback and I'll start making changes on this! I'm agreeing with all of the points on comparing versions within the hook so will start with that too. And maybe adding a few responses to feedback in the process? We'll see.

zimeg

🗒️ A few notes on changes to HTTP error handling!

zimeg · 2024-03-22T22:35:03Z

src/doctor.ts

+  try {
+    const metadataURL = "https://api.slack.com/slackcli/metadata.json";
+    const response = await fetch(metadataURL);
+    if (!response.ok || response.status !== 200) {


The response.status !== 200 protects against successful but unexpected responses, but maybe we want to strengthen this?

The actual fetch follows redirects which I think is good for future compatibility, but that means this can be redirected (302) to a successful page (200) which is not the right page. For instance, a login page. Then the result is this:

zimeg · 2024-03-22T22:40:36Z

src/doctor.ts

+const getHostedDenoRuntimeVersion = async (): Promise<RuntimeDetails> => {
+  try {
+    const metadataURL = "https://api.slack.com/slackcli/metadata.json";
+    const response = await fetch(metadataURL);


This can throw if the hostname can't be found. I hope that's rare because it doesn't seem obvious how we could catch this...

Here's the stacktrace:

filmaj

Looks pretty good but my general question is: how does leveraging the protocol's log/warn/error methods to pump error/diagnostic info back to the CLI impact how we want to use this hook? Particularly for the failure modes, e.g. the response is not what we expect from a remote URL, do we want to include the unexpected response payload (could be JSON, or weird XML, or something else) as part of the hook's response? Or does sending it over the diagnostic line back to the CLI to let it handle that kind of information differently (by e.g. writing to the debug log) clean the output up? I just don't know if we should be showing HTTP status codes and that kind of thing in the doctor output.

In any case this is all up for debate - WDYT?

EDIT: and sorry for the back and forth here, it's not until I saw the error details show up in-line with the doctor output did I remember we have a whole-ass protocol designed for exchanging this kind of information available to us as well. I know it can be frustrating getting things over the finish line but the introduction of a new hook is a particularly good time to be extra-detail-oriented.

zimeg · 2024-03-25T19:42:26Z

Or does sending it over the diagnostic line back to the CLI to let it handle that kind of information differently (by e.g. writing to the debug log) clean the output up? I just don't know if we should be showing HTTP status codes and that kind of thing in the doctor output.

@filmaj I'm liking the idea of sending this back as diagnostic info much better! It lets us send waaay more detail without cluttering the doctor output. And the back-and-forth is no problem. I appreciate the thoroughness 🙌

I'll make updates to log HTTP errors with protocol.warn (?) but am wondering if a follow up in the CLI is necessary?

Right now both stdout (protocol.log) and stderr (protocol.warn and protocol.error) are printed directly as info, but perhaps stderr is treated as debug? I worry errors might be missed in regular output, but not sure how three output types on two streams can be directed in a way that makes sense...

This is what the output with `protocol.warn` looks like now - no explicit error for a failed HTTP request

Note: The only other protocol.* right now (that isn't .respond) is protocol.error in build.ts.

filmaj · 2024-03-25T20:02:07Z

I'll make updates to log HTTP errors with protocol.warn (?) but am wondering if a follow up in the CLI is necessary?

The latest protocol implementation has the CLI treat warn, log and error the same, so in practice no difference between which of these methods should be used.

Ah yes, that's right, the CLI just streams the diagnostic info as a lesser-impact-colour text. So I suppose that puts the onus on the SDK hook implementation to use that in whatever way it sees fit - that is, the SDK hook should format what it passes to the protocol logging methods in a way that would make the output on the CLI side presentable.

I trust you! What do you think feels right in this context?

zimeg · 2024-03-25T20:13:22Z

Since I imagine (hope) HTTP errors will be uncommon I think a `warn` with some leading detail would be most useful.

If we want to hold off on making diagnostic info debug output, I think we can still improve the formatting by not trimming all leading spaces in the error outputs 😅

Perhaps like this? 🤔

filmaj · 2024-03-25T20:26:40Z

The latter looks good to me!

zimeg · 2024-03-25T22:30:06Z

@filmaj @WilliamBergamin and all other kind reviewers, here's a collection of outputs and error states for easy review:

Common cases

Upstream versions are found and match the system versions

System version is later than the upstream version

Upstream version is later than the system version

HTTP errors

The "project runtime" section in the doctor output will match the first common case and no error will be noted in the doctor command. Just these outputs:

Hostname cannot be found

Metadata file cannot be found

Deno runtime version cannot be found

Response JSON is not valid JSON

Notes

I'll begin making the CLI output changes now to preserve spacing like these HTTP errors, but please let me know if you think anything should be changed! 🪝 😄

filmaj

LGTM!

WilliamBergamin

This looks good 💯

zimeg · 2024-03-26T17:27:28Z

@filmaj @WilliamBergamin really appreciate all of the feedback and reviews. Many exciting changes happened here. Y'all rock 🪨 🚀

zimeg added 5 commits February 23, 2024 17:16

feat: provide versions of the execution environment with a doctor hook

24d4cda

feat: collect minimum hosted deno runtime version from metadata

936dcf7

feat: provide notice of a runtime version mismatch for deployed apps

5de5669

feat: include the doctor hook in the get-hooks response

a580ff3

test: verify expected behavior and output values in many cases

a8f80e7

zimeg added enhancement New feature or request semver:minor requires a minor version number bump labels Mar 21, 2024

zimeg self-assigned this Mar 21, 2024

zimeg requested a review from a team as a code owner March 21, 2024 00:04

zimeg added 3 commits March 20, 2024 17:08

docs: note the doctor hook as a supported script

e2385b9

docs(fix): alphabetics applied to the list of supported hooks

8cf422a

docs(fix): include all supported hooks in the list of scripts

1f28a44

WilliamBergamin reviewed Mar 21, 2024

View reviewed changes

filmaj suggested changes Mar 21, 2024

View reviewed changes

zimeg added 8 commits March 21, 2024 11:49

refactor: move checks for newer semversions to utilities

fea6cef

docs: specify the type string a version is extracted from

d8e574d

test: remove await for synchronous code

be8acb7

feat: compare upstream runtime version in the hook

f5429fe

refactor: type runtime details separately from runtime versions

96374f3

feat: remove the returned minimum version from hook responses

6045d51

feat: include http response code in failed metadata requests

c5c167c

feat: include details of missing metadata in hook output

30ce796

zimeg commented Mar 22, 2024

View reviewed changes

zimeg requested review from WilliamBergamin and filmaj March 22, 2024 22:44

filmaj suggested changes Mar 25, 2024

View reviewed changes

feat: log errors with http as warnings using the logger protocol

b81f76f

zimeg requested a review from filmaj March 25, 2024 22:30

filmaj approved these changes Mar 26, 2024

View reviewed changes

WilliamBergamin approved these changes Mar 26, 2024

View reviewed changes

zimeg merged commit 87aaac3 into main Mar 26, 2024
4 checks passed

zimeg deleted the feat-doctor-hook branch March 26, 2024 17:27

zimeg restored the feat-doctor-hook branch July 17, 2024 07:58

zimeg deleted the feat-doctor-hook branch July 17, 2024 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: return runtime versions used by the application with a doctor hook #81

feat: return runtime versions used by the application with a doctor hook #81

zimeg commented Mar 21, 2024 •

edited

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading

WilliamBergamin left a comment

WilliamBergamin Mar 21, 2024

filmaj Mar 21, 2024

zimeg Mar 21, 2024

zimeg Mar 21, 2024

filmaj Mar 21, 2024

zimeg Mar 22, 2024

WilliamBergamin Mar 25, 2024

filmaj left a comment

filmaj Mar 21, 2024

zimeg Mar 22, 2024

filmaj Mar 25, 2024

filmaj Mar 21, 2024

zimeg Mar 22, 2024

filmaj Mar 25, 2024

filmaj Mar 21, 2024

filmaj Mar 21, 2024

zimeg commented Mar 21, 2024

zimeg left a comment

zimeg Mar 22, 2024

zimeg Mar 22, 2024

filmaj left a comment •

edited

Loading

zimeg commented Mar 25, 2024

filmaj commented Mar 25, 2024

zimeg commented Mar 25, 2024

filmaj commented Mar 25, 2024

zimeg commented Mar 25, 2024

filmaj left a comment

WilliamBergamin left a comment

zimeg commented Mar 26, 2024

feat: return runtime versions used by the application with a doctor hook #81

feat: return runtime versions used by the application with a doctor hook #81

Conversation

zimeg commented Mar 21, 2024 • edited Loading

Summary

Testing

Notes

Requirements

codecov bot commented Mar 21, 2024 • edited Loading

Codecov Report

WilliamBergamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimeg commented Mar 21, 2024

zimeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment • edited Loading

Choose a reason for hiding this comment

zimeg commented Mar 25, 2024

filmaj commented Mar 25, 2024

zimeg commented Mar 25, 2024

filmaj commented Mar 25, 2024

zimeg commented Mar 25, 2024

Common cases

HTTP errors

Notes

filmaj left a comment

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

zimeg commented Mar 26, 2024

zimeg commented Mar 21, 2024 •

edited

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading

filmaj left a comment •

edited

Loading