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

filter out last applied config deploy annotations #857

Closed

Conversation

tariq1890
Copy link
Contributor

This is the near-term fix for the issue reported in #854.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 3, 2019
@tariq1890
Copy link
Contributor Author

cc @lilic @brancz

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

This would just filter out the annotation_kubectl_kubernetes_io_last_applied_configuration label from the kube_deployment_annotations metric, but this label will be present in all the resources that can be created/updated with the kubectl/oc apply command. The apply command is the one that sets this annotation, see https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#how-to-create-objects

For example I am seeing it in the namespace as well:

kube_namespace_annotations{namespace="kube-system",annotation_kubectl_kubernetes_io_last_applied_configuration="{\"apiVersion\":\"v1\",\"kind\":\"Namespace\",\"metadata\":{\"annotations\":{},\"name\":\"kube-system\"}}\n"} 1

So we sadly need to filter this out from all the kube_*_annotations metrics.

@tariq1890
Copy link
Contributor Author

Thanks for the review @lilic .

internal/store/utils.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 4, 2019
@lilic
Copy link
Member

lilic commented Aug 5, 2019

I think we wanted this to go in the 1.7 branch? Should we do that first or cherry-pick it out?

@brancz brancz changed the base branch from master to release-1.7 August 5, 2019 07:29
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 5, 2019
@brancz
Copy link
Member

brancz commented Aug 5, 2019

I changed the base branch. This needs a rebase now.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

lgtm after rebase

@lilic
Copy link
Member

lilic commented Aug 5, 2019

So we (@brancz and I) found a few more annotations that kubernetes applies in the metrics, so we decided to just revert the annotations feature altogether. We cannot know all of the kubernetes annotations and what might potently be in there so reverting the feature might be best right now. Will do a PR for the revert.

We should rethink the annotation metrics feature later on, will open an issue for this.

@tariq1890 tariq1890 closed this Aug 5, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 5, 2019
@tariq1890 tariq1890 reopened this Aug 5, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tariq1890

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2019
@k8s-ci-robot
Copy link
Contributor

@tariq1890: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tariq1890 tariq1890 closed this Aug 5, 2019
@2rs2ts
Copy link

2rs2ts commented Aug 6, 2019

Might I suggest using a prefix you define for the annotations to add? Like kubernetes.io/kube-state-metrics/<tag name>?

I'd recommend drawing some inspiration from Datadog's autodiscovery annotations: https://docs.datadoghq.com/agent/autodiscovery/integrations/?tab=kubernetespodannotations#configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants