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

Add Prometheus metrics validation #1575

Merged
merged 7 commits into from
May 24, 2023
Merged

Add Prometheus metrics validation #1575

merged 7 commits into from
May 24, 2023

Conversation

doniacld
Copy link
Contributor

@doniacld doniacld commented May 4, 2023

Motivation

Add the possibility to verify metrics from the Cilium agent pod and validate it has the expected behaviour before and after an action.

Changes

  • commit 1 Add a k8s dialer to be able to port forward /metrics endpoint outside of the pod
  • commit 2 Add the logic to call /metrics endpoint and parse the metrics to retrieve them into a nice usable open metrics format
  • commit 3 policy.go is a pretty long file now, let's move Result with its associated methods in its own file.
  • commit 4 Make the call to collect metrics in the action
  • commit 5 Validate the metrics by defining a new type metricsCompareFunc which will compare metrics in a generic way.
  • commit 6 Add a new scenario very basic with a simple metric to verify the feature is working

Usage

Implement the operation you want to check on your metrics on Result:

func (r Result) ExpectMetricsIncrease(source MetricsSource, metrics ...string) Result {

Then use it directly in your expectations by passing a MetricSource type (cilium-agent, cilium-operator...) and the name of the metric you want to check:

return check.ResultOK.ExpectMetricsIncrease(ct.CiliumAgentMetrics(), "cilium_forward_count_total")

@doniacld doniacld requested review from a team as code owners May 4, 2023 08:53
@doniacld doniacld temporarily deployed to ci May 4, 2023 08:53 — with GitHub Actions Inactive
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.

Awesome work, thanks a lot! I've left some feedback regarding the open TODOs.

connectivity/check/action.go Outdated Show resolved Hide resolved
connectivity/check/policy.go Outdated Show resolved Hide resolved
connectivity/check/policy.go Outdated Show resolved Hide resolved
connectivity/check/policy.go Outdated Show resolved Hide resolved
k8s/dialer.go Outdated Show resolved Hide resolved
connectivity/check/check.go Outdated Show resolved Hide resolved
@doniacld doniacld marked this pull request as draft May 5, 2023 08:25
@doniacld doniacld temporarily deployed to ci May 8, 2023 10:17 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 8, 2023 10:48 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 8, 2023 20:33 — with GitHub Actions Inactive
@doniacld doniacld marked this pull request as ready for review May 11, 2023 08:17
@doniacld doniacld temporarily deployed to ci May 11, 2023 09:18 — with GitHub Actions Inactive
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.

Awesome! High-level this looks good to me. I did leave some minor feedback that should be addressed before we can merge. Thanks for working on this!

k8s/dialer.go Outdated Show resolved Hide resolved
connectivity/check/metrics.go Outdated Show resolved Hide resolved
connectivity/check/metrics.go Outdated Show resolved Hide resolved
k8s/dialer.go Show resolved Hide resolved
connectivity/check/metrics.go Show resolved Hide resolved
connectivity/check/policy.go Show resolved Hide resolved
connectivity/check/result.go Outdated Show resolved Hide resolved
connectivity/check/action.go Outdated Show resolved Hide resolved
connectivity/tests/pod.go Show resolved Hide resolved
connectivity/check/metrics.go Outdated Show resolved Hide resolved
@doniacld doniacld temporarily deployed to ci May 15, 2023 08:07 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 15, 2023 08:26 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 15, 2023 08:43 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 15, 2023 08:51 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 15, 2023 16:31 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 16, 2023 08:09 — with GitHub Actions Inactive
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.

Awesome, overall looks good to me! A couple minor nits remaining

connectivity/check/metrics.go Outdated Show resolved Hide resolved
connectivity/check/result.go Outdated Show resolved Hide resolved
@doniacld doniacld temporarily deployed to ci May 18, 2023 08:37 — with GitHub Actions Inactive
@doniacld doniacld temporarily deployed to ci May 18, 2023 08:46 — with GitHub Actions Inactive
Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld temporarily deployed to ci May 22, 2023 08:30 — with GitHub Actions Inactive
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! Two minor (non-blocking) nits remaining

connectivity/check/action.go Outdated Show resolved Hide resolved
connectivity/check/metrics.go Show resolved Hide resolved
@doniacld doniacld marked this pull request as draft May 22, 2023 12:38
@doniacld doniacld temporarily deployed to ci May 22, 2023 14:22 — with GitHub Actions Inactive
Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld temporarily deployed to ci May 22, 2023 14:45 — with GitHub Actions Inactive
Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld temporarily deployed to ci May 22, 2023 14:58 — with GitHub Actions Inactive
… total request

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld temporarily deployed to ci May 22, 2023 16:17 — with GitHub Actions Inactive
@doniacld doniacld marked this pull request as ready for review May 23, 2023 08:23
@doniacld doniacld requested review from a team as code owners May 23, 2023 08:23
@doniacld doniacld requested a review from brlbil May 23, 2023 08:23
Copy link
Contributor

@brlbil brlbil left a comment

Choose a reason for hiding this comment

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

The CI part is 👍 Awesome work, thanks a lot!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 23, 2023
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Nice work!

@tklauser tklauser merged commit 1d5f54a into cilium:main May 24, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants