weaver icon indicating copy to clipboard operation
weaver copied to clipboard

Added metrics, profile, purge status cmd to ssh

Open spicavigo opened this issue 2 years ago • 6 comments

I have limited understanding of the codebase, however, this does seem to work.

There are a few styling issues that annoyed me but I decided not to work on those so as to not take the focus away from what this PR does. The issues being

  1. multi defines all the spec within multi.go while ssh does not
  2. multi uses function in deploy.go for registry while ssh uses function declared in impl/manager.go for registry
  3. There is an impl folder in ssh but not in multi
  4. There seem to be no tests for ssh

Apart from these, there is no documentation for ssh.

I am not sure if above (except documentation) is by design or just due to how fast things are getting implemented. If its the latter, then perhaps I (or someone else) can start working on these)

spicavigo avatar Jun 28 '23 05:06 spicavigo

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jun 28 '23 05:06 google-cla[bot]

@rgrandl Should I remove pb.go files from this PR? It seems the only change in those files is a comment of this sort

"- // protoc v3.21.12" "+ // protoc v4.23.3"

spicavigo avatar Jun 29 '23 05:06 spicavigo

It's totally fine because the generated proto files should be updated as well at head.

rgrandl avatar Jun 29 '23 06:06 rgrandl

@spicavigo thanks for your contribution. I think some of the commands are not yet supported by the SSH deployer. Could you please check which ones are actually working (e.g., weaver ssh metrics doesn't show anything, which means we might need some small tweaks in the ssh deployer to make it work).

If you prefer, you can remove from this PR the ones who are not supported yet, or you can take a look at the ones not supported yet and try to implement a fix as well.

rgrandl avatar Jun 29 '23 07:06 rgrandl

Hi @rgrandl . I have not verified it from the code, but metrics command does seem to provide some data. Verification is necessary to ensure its the correct data. Here is my output from metrics command after sending a few curl request to my service

Screenshot 2023-06-29 at 12 53 44 PM

Status: Screenshot 2023-06-29 at 12 55 00 PM

Purge: Screenshot 2023-06-29 at 12 55 38 PM

I can of course verify output of each of these command by going through code or create an AI (is there a bug tracking setup for service weaver?) on me to do that post this PR. LMK what you prefer.

spicavigo avatar Jun 29 '23 19:06 spicavigo

It would be good to verify, at least by visual inspection, that all of these commands work, before checking the code in. To me, it looks like the metrics code is functional. No need to do anything beyond visual inspection. (e.g., collect traces and look at them in Perfetto etc.)

spetrovic77 avatar Jun 30 '23 17:06 spetrovic77