Initial draft for sending skaffold metrics using metadata event by tejal29 · Pull Request #3966 · GoogleContainerTools/skaffold

tejal29 · 2020-04-17T00:44:42Z

Fixes: #3967
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

In this Pr,

Added a Skaffold Metadata proto
- This proto contains Build, Deploy metadata like number of artifacts, builders and deployers used etc.
- an additional kay value pair to stick any data required for analysis. (This is an experimental section and will be moved to a first class field

User facing changes (remove if N/A)
No.
For IDEs, Ides will now see skaffold metadata in MetaEvent

{"result":{"timestamp":"2020-04-17T18:22:46.325297Z","
event":{"metaEvent":{"entry":"Starting Skaffold: \u0026{Version:v1.6.0-82-gbfe9b97e4-dirty ConfigVersion:skaffold/v2beta2 GitVersion: GitCommit:bfe9b97e4ced1fb91e9cb768b49879a63202eaec GitTreeState:dirty BuildDate:2020-04-17T11:21:55Z GoVersion:go1.14.1 Compiler:gc Platform:darwin/amd64}","
metadata":{
  "build":{"numberOfArtifacts":1,"builders":[{"type":"DOCKER","count":1}],"type":"LOCAL"},
  "deploy":{"deployers":[{"type":"KUBECTL","count":1}],"cluster":"GKE"}}}}}}

Follow-up Work (remove if N/A)
yes! Will add unit tests.

codecov · 2020-04-17T00:52:02Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f70a96c). Click here to learn what that means.
The diff coverage is 92.77%.

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/cmd.go	`68.21% <ø> (ø)`
pkg/skaffold/event/util.go	`91.17% <91.17%> (ø)`
pkg/skaffold/event/event.go	`93.13% <100.00%> (ø)`
pkg/skaffold/runner/new.go	`70.00% <100.00%> (ø)`
pkg/skaffold/deploy/helm.go	`79.12% <0.00%> (ø)`
pkg/skaffold/test/structure/structure.go	`100.00% <0.00%> (ø)`
pkg/skaffold/runner/runner.go	`0.00% <0.00%> (ø)`
pkg/skaffold/build/build.go	`0.00% <0.00%> (ø)`
pkg/skaffold/build/buildpacks/metadata.go	`100.00% <0.00%> (ø)`
pkg/skaffold/runner/cleanup.go	`0.00% <0.00%> (ø)`
... and 293 more

nkubala

looks like a good starting point! i suspect we'll want to add more information to these events but that can come later.

pkg/skaffold/event/event.go

proto/skaffold.proto

pkg/skaffold/event/event.go

nkubala · 2020-04-21T21:40:28Z

proto/skaffold.proto

+    KUBECTL = 3;
+}
+
+enum ClusterType {


seems like we might want to list out some of the other common k8s cluster providers here. otherwise users are going to see "Cluster Type: Other" which doesn't seem that useful?

so for Privacy concerns, we can't make a distinction which "Other" or competitor cloud provider service users are using :)

Can we differentiate with other opensource distributions? (k3d, microk8s, ...?)

pkg/skaffold/event/util.go

tejal29 · 2020-04-22T05:19:39Z

suspect we'll want to add more information to these events but that can come later.

yes. I am thinking of adding those as we see appropriate later in the free "key/value" map. Also, note for every field we add, we need to go trough approval process. For now, these should be apt for metrics we are interested in short term.

briandealwis

LGTM. But do we also want to record the build command (dev, debug, build, deploy)?

briandealwis · 2020-04-23T18:18:01Z

pkg/skaffold/event/util.go

+	"github.com/GoogleContainerTools/skaffold/proto"
+)
+
+func initializeMetadata(p latest.Pipeline, kubeContext string) *proto.Metadata {


WDYT of including a hash of the pipeline? That would let us tell if the user re-ran or if they made a change.

oh this is an interesting idea. you could even just translate that to a isPipelineChange boolean or something.

briandealwis · 2020-04-23T18:24:29Z

pkg/skaffold/event/util_test.go

+					Deployers: []*proto.DeployMetadata_Deployer{{Type: proto.DeployerType_KUSTOMIZE, Count: 1}},
+				},
+			},
+		},


Worth adding a no-builders no-deployers case?

briandealwis · 2020-04-23T18:29:57Z

proto/skaffold.proto

+    KUBECTL = 3;
+}
+
+enum ClusterType {


Can we differentiate with other opensource distributions? (k3d, microk8s, ...?)

tejal29 · 2020-04-23T18:41:55Z

LGTM. But do we also want to record the build command (dev, debug, build, deploy)?

no. The IDEs will provide invokation details. MetaEvent will only provide information on Skaffold Pipeline Metadata.

addressed changes!

nkubala

LGTM, I think @briandealwis had some good ideas for enhancements though. those can come in follow up PRs though

balopat

we need to provide proper notices for anything metrics related

tejal29 requested review from balopat, dgageot and nkubala as code owners April 17, 2020 00:44

googlebot added the cla: yes label Apr 17, 2020

pull-request-size bot added the size/XXL label Apr 17, 2020

tejal29 force-pushed the add_metadata branch 2 times, most recently from 0ec0025 to aefa9e2 Compare April 17, 2020 21:26

nkubala previously requested changes Apr 21, 2020

View reviewed changes

tejal29 closed this Apr 23, 2020

tejal29 reopened this Apr 23, 2020

tejal29 added 6 commits April 22, 2020 21:33

Initial draft for sending skaffold metrics using metadata event

b45778c

fix lint and gofmt

c3777ec

remove fmt.Println

b5f1d00

fix tests. Do not user 0 as value since it is omitted

d09ef21

code review comments

0631cf5

fix rebase from master

d64f159

tejal29 force-pushed the add_metadata branch from 3d983fd to d64f159 Compare April 23, 2020 04:40

briandealwis approved these changes Apr 23, 2020

View reviewed changes

nkubala approved these changes Apr 23, 2020

View reviewed changes

balopat suggested changes Apr 23, 2020

View reviewed changes

tejal29 merged commit eab0d15 into GoogleContainerTools:master Apr 23, 2020

fmotrifork mentioned this pull request May 6, 2020

skaffold 1.9.0 fishworks/fish-food#713

Merged

tejal29 deleted the add_metadata branch April 15, 2021 07:35

Conversation

tejal29 commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkubala Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

tejal29 Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

briandealwis Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tejal29 commented Apr 22, 2020

Uh oh!

briandealwis left a comment

Choose a reason for hiding this comment

Uh oh!

briandealwis Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

nkubala Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

briandealwis Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

briandealwis Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

tejal29 commented Apr 23, 2020

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tejal29 commented Apr 17, 2020 •

edited

Loading

codecov bot commented Apr 17, 2020 •

edited

Loading