Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added metrics, profile, purge status cmd to ssh #423

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spicavigo
Copy link

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)

@google-cla
Copy link

google-cla bot commented Jun 28, 2023

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.

@spicavigo
Copy link
Author

spicavigo commented Jun 29, 2023

@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"

@rgrandl
Copy link
Collaborator

rgrandl commented Jun 29, 2023

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

@rgrandl
Copy link
Collaborator

rgrandl commented Jun 29, 2023

@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.

@spicavigo
Copy link
Author

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.

@spetrovic77
Copy link
Contributor

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants