-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Initial draft for sending skaffold metrics using metadata event #3966
Conversation
Codecov Report
|
0ec0025
to
aefa9e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a good starting point! i suspect we'll want to add more information to these events but that can come later.
KUBECTL = 3; | ||
} | ||
|
||
enum ClusterType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so for Privacy concerns, we can't make a distinction which "Other" or competitor cloud provider service users are using :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we differentiate with other opensource distributions? (k3d, microk8s, ...?)
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But do we also want to record the build command (dev, debug, build, deploy)?
"github.com/GoogleContainerTools/skaffold/proto" | ||
) | ||
|
||
func initializeMetadata(p latest.Pipeline, kubeContext string) *proto.Metadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT of including a hash of the pipeline? That would let us tell if the user re-ran or if they made a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh this is an interesting idea. you could even just translate that to a isPipelineChange
boolean or something.
Deployers: []*proto.DeployMetadata_Deployer{{Type: proto.DeployerType_KUSTOMIZE, Count: 1}}, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a no-builders no-deployers case?
KUBECTL = 3; | ||
} | ||
|
||
enum ClusterType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we differentiate with other opensource distributions? (k3d, microk8s, ...?)
no. The IDEs will provide invokation details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think @briandealwis had some good ideas for enhancements though. those can come in follow up PRs though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to provide proper notices for anything metrics related
Fixes: #3967
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs
Description
In this Pr,
Metadata
protoBuild
,Deploy
metadata like number of artifacts, builders and deployers used etc.User facing changes (remove if N/A)
No.
For IDEs, Ides will now see skaffold metadata in
MetaEvent
Follow-up Work (remove if N/A)
yes! Will add unit tests.