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

scout/usage: add --docker flag to scout usage tool #985

Merged
merged 9 commits into from
May 25, 2023

Conversation

jasonhawkharris
Copy link
Contributor

Test plan

Tested manually with healthy and unhealthy docker sourcegraph instance.

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing a bit of context here, what is the relation of this tool and the docker stats command?
This is what it would give:

 ~ docker stats --no-stream --no-trunc --all
CONTAINER ID                                                       NAME                 CPU %     MEM USAGE / LIMIT     MEM %     NET I/O       BLOCK I/O   PIDS
4844cb8c6fd16f57f69290197f8eaa6b8f4e7a340148a8ba3bc3ad1c4139b972   syntax-highlighter   0.00%     34.92MiB / 7.765GiB   0.44%     1.51kB / 0B   0B / 0B     14
6696bf320826e206b910284e2f4b8b0bf8d2d45105a343d347383cdbbaf2c89e   lago-front           0.00%     0B / 0B               0.00%     0B / 0B       0B / 0B     0

internal/scout/usage/docker.go Outdated Show resolved Hide resolved
internal/scout/usage/docker.go Outdated Show resolved Hide resolved
@jasonhawkharris
Copy link
Contributor Author

jasonhawkharris commented May 25, 2023

@eseliger long and short of it is that I just haven't found a way to get cpu/memory usage from the goclient for the docker API. The dockerstats library solves this issue, and does so quicker than if I were to just wrap the command in an exec function.

@eseliger
Copy link
Member

Is performance a huge problem here? The docker stats command also allows structured output in JSON. Just trying to avoid reinventing the wheel here if there's something suitable already that would work similarly, there are likely edge-cases that docker CLI people have thought of already that we haven't.
If there's good reasons for going with this completely custom implementation, happy to take that, just making sure we consider our options and make sure we need to write and maintain as much code as needed and as little as possible :)

@jasonhawkharris
Copy link
Contributor Author

Performance is not a huge problem here. You're right.

Found a way to do this with the docker client. Requesting re-review.

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, just to clarify - what is this going to be used for that docker stats could not do? Is this for envs where docker cli isn't available? Is this meant to generate a more elaborate report?

internal/scout/usage/docker.go Outdated Show resolved Hide resolved
internal/scout/usage/docker.go Outdated Show resolved Hide resolved
@jasonhawkharris
Copy link
Contributor Author

src scout usage works on a Kubernetes deployment and returns current usages. The usage command, in particular, is more useful for Kubernetes deployments.

However, adding the --docker flag will allow it to work on docker deployments as well. It's less needed because of the docker stats command, but I wanted to have it as an option for our support/TA/CE folks because this command takes you to a TUI that allows the user to easily export this information and send it back to us.

Happy to hop on a call if you'd like to see this in real time.

@eseliger
Copy link
Member

Thanks, that's the bit of context I was missing :)

@jasonhawkharris jasonhawkharris merged commit d9d8ca6 into main May 25, 2023
@jasonhawkharris jasonhawkharris deleted the jhh/scout-usage-docker-flag branch May 25, 2023 19:39
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.

2 participants