Add a `skaffold diagnose` command by dgageot · Pull Request #1109 · GoogleContainerTools/skaffold

dgageot · 2018-10-08T09:02:58Z

balopat · 2018-10-08T15:55:10Z

cmd/skaffold/app/cmd/diagnose.go

+			return errors.Wrap(err, "listing artifact dependencies")
+		}
+
+		fmt.Fprintln(out, "Type:", typeOfArtifact(artifact))


would be nice to have the artifact name here as well - on Ahmet's microservices demo this becomes very hard to follow. But - if someone wants to anonymize before sharing, maybe have an "--anonymize" mode where it prints no names but at least the artifact indexes?

Skaffold version: c2b06e54ebd4eabdccf7101d21520a9dae5e050d Configuration version: skaffold/v1alpha4 Number of artifacts: 10 Running a diagnostic on artifacts and their dependencies: Type: Docker - Dependencies to watch: 27 - Time to list dependencies: 18.67081ms - Size of the Docker context: 1940480 - Time to compute mTimes for all dependencies: 204.849µs Type: Docker - Dependencies to watch: 9 - Time to list dependencies: 13.525253ms - Size of the Docker context: 121856 - Time to compute mTimes for all dependencies: 68.634µs Type: Docker - Dependencies to watch: 7 - Time to list dependencies: 11.399753ms - Size of the Docker context: 85504 - Time to compute mTimes for all dependencies: 61.37µs Type: Docker - Dependencies to watch: 8 - Time to list dependencies: 7.941914ms - Size of the Docker context: 95232 - Time to compute mTimes for all dependencies: 64.555µs Type: Docker - Dependencies to watch: 32 - Time to list dependencies: 10.389932ms - Size of the Docker context: 1309184 - Time to compute mTimes for all dependencies: 225.894µs Type: Docker - Dependencies to watch: 4 - Time to list dependencies: 9.034657ms - Size of the Docker context: 7680 - Time to compute mTimes for all dependencies: 34.428µs Type: Docker - Dependencies to watch: 11 - Time to list dependencies: 6.168579ms - Size of the Docker context: 92672 - Time to compute mTimes for all dependencies: 80.195µs Type: Docker - Dependencies to watch: 9 - Time to list dependencies: 11.701616ms - Size of the Docker context: 114176 - Time to compute mTimes for all dependencies: 145.571µs Type: Docker - Dependencies to watch: 8 - Time to list dependencies: 5.532876ms - Size of the Docker context: 94208 - Time to compute mTimes for all dependencies: 52.45µs Type: Docker - Dependencies to watch: 10 - Time to list dependencies: 13.208922ms - Size of the Docker context: 114176 - Time to compute mTimes for all dependencies: 87.346µs Second run (that benefits from cache): Type: Docker - Dependencies to watch: 27 - Time to list dependencies: 1.273089ms - Size of the Docker context: 1940480 - Time to compute mTimes for all dependencies: 151.649µs Type: Docker - Dependencies to watch: 9 - Time to list dependencies: 734.717µs - Size of the Docker context: 121856 - Time to compute mTimes for all dependencies: 54.673µs Type: Docker - Dependencies to watch: 7 - Time to list dependencies: 680.493µs - Size of the Docker context: 85504 - Time to compute mTimes for all dependencies: 39.901µs Type: Docker - Dependencies to watch: 8 - Time to list dependencies: 460.873µs - Size of the Docker context: 95232 - Time to compute mTimes for all dependencies: 45.44µs Type: Docker - Dependencies to watch: 32 - Time to list dependencies: 774.482µs - Size of the Docker context: 1309184 - Time to compute mTimes for all dependencies: 181.169µs Type: Docker - Dependencies to watch: 4 - Time to list dependencies: 496.26µs - Size of the Docker context: 7680 - Time to compute mTimes for all dependencies: 26.045µs Type: Docker - Dependencies to watch: 11 - Time to list dependencies: 643.966µs - Size of the Docker context: 92672 - Time to compute mTimes for all dependencies: 63.188µs Type: Docker - Dependencies to watch: 9 - Time to list dependencies: 746.828µs - Size of the Docker context: 114176 - Time to compute mTimes for all dependencies: 51.068µs Type: Docker - Dependencies to watch: 8 - Time to list dependencies: 390.799µs - Size of the Docker context: 94208 - Time to compute mTimes for all dependencies: 49.544µs Type: Docker - Dependencies to watch: 10 - Time to list dependencies: 792.987µs - Size of the Docker context: 114176 - Time to compute mTimes for all dependencies: 58.997µs

or maybe a --quiet mode that only prints the essential information?

I'm going to improve the output

balopat · 2018-10-08T15:59:59Z

cmd/skaffold/app/cmd/diagnose.go

+		return errors.Wrap(err, "running diagnostic on artifacts")
+	}
+
+	fmt.Fprintln(out, "\nSecond run (that benefits from cache):")


an idea: what if we list the result of the runs for each metric, instead of printing a full run?
i.e.:

Number of artifacts: 2 Running a diagnostic on artifacts and their dependencies: Type: Docker - Dependencies to watch: 2 - Time to list dependencies: 4.809915ms - Size of the Docker context: 3072 - Time to compute mTimes for all dependencies: 19.012µs Type: Docker - Dependencies to watch: 2 - Time to list dependencies: 454.866µs - Size of the Docker context: 3072 - Time to compute mTimes for all dependencies: 16.534µs Second run (that benefits from cache): Type: Docker - Dependencies to watch: 2 - Time to list dependencies: 396.262µs - Size of the Docker context: 3072 - Time to compute mTimes for all dependencies: 16.35µs Type: Docker - Dependencies to watch: 2 - Time to list dependencies: 305.815µs - Size of the Docker context: 3072 - Time to compute mTimes for all dependencies: 29.689µs

it would be this:

Number of artifacts: 2 Running a diagnostic on artifacts and their dependencies: Type: Docker - Dependencies to watch: 2 - Time to list dependencies: 4.809915ms, 396.262µs (cache) - Size of the Docker context: 3072 - Time to compute mTimes for all dependencies: 19.012µs, 16.35µs (cache) Type: Docker - Dependencies to watch: 2 - Time to list dependencies: 454.866µs, 305.815µs (cached) - Size of the Docker context: 3072 - Time to compute mTimes for all dependencies: 16.534µs, 29.689µs (cached)

I'll +1 this, the output of this can get pretty large for bigger projects

balopat · 2018-10-08T16:01:06Z

cmd/skaffold/app/cmd/diagnose.go

+				return errors.Wrap(err, "computing the size of the Docker context")
+			}
+
+			fmt.Fprintln(out, " - Size of the Docker context:", size)


do we want a human readable size format here, or there's a reason you keep bytes? Even if so, I think a bytes unit can be useful.

there are packages out there that will do automatic "humanization" of byte sizes for you, e.g. https://github.com/dustin/go-humanize

nkubala · 2018-10-08T17:52:30Z

cmd/skaffold/app/cmd/diagnose.go

+		return errors.Wrap(err, "running diagnostic on artifacts")
+	}
+
+	fmt.Fprintln(out, "\nSecond run (that benefits from cache):")


I'll +1 this, the output of this can get pretty large for bigger projects

nkubala · 2018-10-08T17:53:20Z

cmd/skaffold/app/cmd/diagnose.go

+			return errors.Wrap(err, "listing artifact dependencies")
+		}
+
+		fmt.Fprintln(out, "Type:", typeOfArtifact(artifact))


or maybe a --quiet mode that only prints the essential information?

nkubala · 2018-10-08T17:56:02Z

cmd/skaffold/app/cmd/diagnose.go

+				return errors.Wrap(err, "computing the size of the Docker context")
+			}
+
+			fmt.Fprintln(out, " - Size of the Docker context:", size)


there are packages out there that will do automatic "humanization" of byte sizes for you, e.g. https://github.com/dustin/go-humanize

nkubala · 2018-10-08T17:56:20Z

cmd/skaffold/app/cmd/diagnose.go

+	return nil
+}
+
+func sizeOfTheDockerContext(a *latest.Artifact) (int64, error) {


nit: sizeOfDockerContext()

nkubala · 2018-10-08T17:58:23Z

cmd/skaffold/app/cmd/diagnose.go

+	return nil
+}
+
+func diagnoseArtifacts(out io.Writer, artifacts []*latest.Artifact) error {


it might also be worth trying to infer some of the builder- and deployer- specific information for each artifact. e.g. docker version for a docker artifact, bazel version for a bazel artifact, kubectl version, helm version, etc.

codecov-io · 2018-10-09T06:22:25Z

Codecov Report

Merging #1109 into master will decrease coverage by 1.17%.
The diff coverage is 5.74%.

@@            Coverage Diff             @@
##           master    #1109      +/-   ##
==========================================
- Coverage   43.54%   42.36%   -1.18%     
==========================================
  Files          84       78       -6     
  Lines        3734     3609     -125     
==========================================
- Hits         1626     1529      -97     
+ Misses       1951     1927      -24     
+ Partials      157      153       -4

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/diagnose.go	`0% <0%> (ø)`
pkg/skaffold/runner/runner.go	`51.47% <0%> (+1.27%)`	⬆️
cmd/skaffold/app/cmd/cmd.go	`0% <0%> (ø)`	⬆️
cmd/skaffold/app/cmd/dev.go	`0% <0%> (ø)`	⬆️
cmd/skaffold/app/cmd/fix.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/watch/changes.go	`97.87% <100%> (ø)`	⬆️
pkg/skaffold/watch/watch.go	`85% <100%> (ø)`	⬆️
pkg/skaffold/util/util.go	`45.09% <0%> (-4%)`	⬇️
pkg/skaffold/deploy/kubectl.go	`51.21% <0%> (-1.17%)`	⬇️
pkg/skaffold/deploy/kubectl/cli.go	`0% <0%> (ø)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01a6ad8...a72da4e. Read the comment docs.

Signed-off-by: David Gageot <david@gageot.net>

Fixes GoogleContainerTools#1094 Signed-off-by: David Gageot <david@gageot.net>

dgageot requested review from balopat, nkubala and r2d4 as code owners October 8, 2018 09:02

balopat reviewed Oct 8, 2018

View reviewed changes

nkubala reviewed Oct 8, 2018

View reviewed changes

dgageot force-pushed the diagnose branch from c2b06e5 to 9a1cb2a Compare October 9, 2018 06:07

dgageot added 2 commits October 10, 2018 18:53

Move flags next to the command

f848d4b

Signed-off-by: David Gageot <david@gageot.net>

Add a skaffold diagnose command

a72da4e

Fixes GoogleContainerTools#1094 Signed-off-by: David Gageot <david@gageot.net>

dgageot force-pushed the diagnose branch from d8c9104 to a72da4e Compare October 10, 2018 16:56

nkubala approved these changes Oct 10, 2018

View reviewed changes

dgageot merged commit 7407bf1 into GoogleContainerTools:master Oct 10, 2018

dgageot deleted the diagnose branch December 28, 2018 07:14

Conversation

dgageot commented Oct 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Oct 9, 2018 •

edited

Loading