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.

Sign up for GitHub

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

Jump to bottom

metrics: Set first retrieved metrics to 0 if absent #1864

Merged
merged 1 commit into from
Jul 27, 2023
Merged

metrics: Set first retrieved metrics to 0 if absent #1864

merged 1 commit into from
Jul 27, 2023

Conversation

doniacld
Copy link
Contributor

@doniacld doniacld commented Jul 25, 2023 •
edited

When a cluster is freshly deployed and no traffic was observed, certain metrics does not appear yet. This commit intents to set to 0 the counters of the MetricFamily if the metric was not present.

Testing on a fresh cluster:

No metrics at the beginning, set to 0:

2023/07/25 10:45:42 [WARNING] metric xxx was not found before the action was run
2023/07/25 10:45:42 [WARNING] metric xxx counters was set to 0

Finally checked the metrics properly:

🐛 Forwarding from [::1]:60476 -> 9967

🐛 Handling connection for 60476

  🐛 checked metrics properly on node kind-worker

  🐛 Finalizing Test external-cilium-dns-proxy
  🐛 Pod kube-system/cilium-dzmnb's current policy revision: 2
  🐛 Pod kube-system/cilium-wmvfn's current policy revision: 2
  ℹ️  📜 Deleting CiliumNetworkPolicy 'dns-via-proxy-for-client' from namespace 'cilium-test'..
  🐛 Pod kind-kind/kube-system/cilium-dzmnb revision > 2
  🐛 Pod kind-kind/kube-system/cilium-wmvfn revision > 2
  🐛 📜 Successfully deleted 1 CiliumNetworkPolicies

✅ All 1 tests (4 actions) successful, 54 tests skipped, 0 scenarios skipped.

@doniacld doniacld requested a review from a team as a code owner July 25, 2023 08:09
@doniacld doniacld requested a review from derailed July 25, 2023 08:09
@doniacld doniacld temporarily deployed to ci July 25, 2023 08:09 — with GitHub Actions Inactive
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from 924c162 to a15d5c1 Compare July 25, 2023 12:42
@doniacld doniacld temporarily deployed to ci July 25, 2023 12:42 — with GitHub Actions Inactive
@doniacld doniacld requested a review from a team July 26, 2023 12:49
michi-covalent
michi-covalent reviewed Jul 26, 2023
View reviewed changes
connectivity/check/metrics.go Outdated Show resolved Hide resolved
connectivity/check/result.go Show resolved Hide resolved
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from a15d5c1 to 12809ee Compare July 27, 2023 11:32
@doniacld doniacld temporarily deployed to ci July 27, 2023 11:32 — with GitHub Actions Inactive
@doniacld doniacld requested a review from gandro July 27, 2023 11:33
gandro
gandro requested changes Jul 27, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

One minor nit (https://github.com/cilium/cilium-cli/pull/1864/files#r1276205954) and one issue with the log package

connectivity/check/result.go Outdated Show resolved Hide resolved
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from 12809ee to 7d6644a Compare July 27, 2023 13:16
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from 7d6644a to b76e763 Compare July 27, 2023 13:19
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from b76e763 to c45b2b5 Compare July 27, 2023 13:20
@doniacld doniacld temporarily deployed to ci July 27, 2023 13:20 — with GitHub Actions Inactive
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from c45b2b5 to d36b8dd Compare July 27, 2023 13:24
@doniacld doniacld temporarily deployed to ci July 27, 2023 13:24 — with GitHub Actions Inactive
@doniacld
metrics: Set first retrieved metrics to 0 if absent
975bc08 
When a cluster is freshly deployed and no traffic was observed, certain metrics
does not appear yet. This commit intents to assume the default value of the
counters is 0 the counters if the metric is absent.

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld force-pushed the pr/doniacld/metric-0-if-absent branch from d36b8dd to 975bc08 Compare July 27, 2023 13:30
@doniacld doniacld temporarily deployed to ci July 27, 2023 13:31 — with GitHub Actions Inactive
michi-covalent
michi-covalent approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

thank you! 🥰

@michi-covalent michi-covalent merged commit 3006aa5 into cilium:main Jul 27, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

@derailed derailed Awaiting requested review from derailed derailed was automatically assigned from cilium/cli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@doniacld @gandro @michi-covalent