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

Enable per limit metrics #113

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Enable per limit metrics #113

merged 1 commit into from
Nov 14, 2023

Conversation

alexsnaps
Copy link
Member

@alexsnaps alexsnaps commented Nov 7, 2023

This builds on the changes in #112 - which lets a RateLimit have a name.

Add the telemetry field to the LimitadorSpec, which currently has two valid options: basic (what it's been so far) and exhaustive, which will result in starting the Limitador server process with the --limit-name-in-labels option, resulting in the two metrics, limited_calls & authorized_calls, to include the limit's name field as label - providing better metrics about what Limit is limiting or allow traffic through.

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2023

Codecov Report

Merging #113 (5645e70) into main (bb16f15) will decrease coverage by 0.21%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
- Coverage   68.80%   68.59%   -0.21%     
==========================================
  Files          14       14              
  Lines         984      987       +3     
==========================================
  Hits          677      677              
- Misses        267      269       +2     
- Partials       40       41       +1     
Flag Coverage Δ
integration 66.06% <ø> (ø)
unit 69.87% <0.00%> (-0.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1alpha1 (u) 100.00% <ø> (ø)
pkg/helpers (u) ∅ <ø> (∅)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 44.19% <ø> (ø)
pkg/limitador (u) 87.68% <0.00%> (-0.78%) ⬇️
controllers (i) 66.06% <ø> (ø)
Files Coverage Δ
api/v1alpha1/limitador_types.go 100.00% <ø> (ø)
pkg/limitador/deployment_options.go 92.10% <0.00%> (-7.90%) ⬇️

@alexsnaps alexsnaps force-pushed the rlp-named-2 branch 3 times, most recently from faf1456 to 859fee3 Compare November 8, 2023 14:04
@alexsnaps alexsnaps force-pushed the rlp-named-2 branch 2 times, most recently from ced91de to e49127f Compare November 8, 2023 14:12
@alexsnaps alexsnaps changed the title Rlp named 2 Enable per limit metrics Nov 8, 2023
@eguzki
Copy link
Contributor

eguzki commented Nov 9, 2023

Verification Steps ✔️

dev setup

make local-setup

Deploy the limitador CR without

k apply -f - <<EOF
---
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  limits:
    - conditions: ["get_toy == 'yes'"]
      max_value: 2
      namespace: toystore-app
      seconds: 30
      variables: []
      name: "toy_get_route"
EOF

Check deployment is correct

kubectl wait --timeout=300s --for=condition=Ready limitador limitador-sample

Should return

limitador.limitador.kuadrant.io/limitador-sample condition met

Check command line

kubectl get deployment/limitador-sample  -o yaml | yq '.spec.template.spec.containers[0].command'

should look like this

- limitador-server
- /home/limitador/etc/limitador-config.yaml
- memory

Let's add the telemetry field

k apply -f - <<EOF
---
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador-sample
spec:
  telemetry: exhaustive
  limits:
    - conditions: ["get_toy == 'yes'"]
      max_value: 2
      namespace: toystore-app
      seconds: 30
      variables: []
      name: "toy_get_route"
EOF

Check command line

kubectl get deployment/limitador-sample  -o yaml | yq '.spec.template.spec.containers[0].command'

should look like this

- limitador-server
- --limit-name-in-labels
- /home/limitador/etc/limitador-config.yaml
- memory

Whether that new command line arg --limit-name-in-labels includes or not the limit's name field as label is out of scope of the operator.

@@ -76,6 +76,9 @@ type LimitadorSpec struct {
// +optional
RateLimitHeaders *RateLimitHeadersType `json:"rateLimitHeaders,omitempty"`

// +optional
Telemetry *Telemetry `json:"telemetry,omitempty"`
Copy link
Contributor

@eguzki eguzki Nov 9, 2023

Choose a reason for hiding this comment

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

Telemetry is maybe too generic name, not very meaningful for the very specific task that is doing: "adding limit names as label in the metrics". Or you are planning to add more command line args like --limit-name-in-labels when the value of the telemtry is exhaustive??

Copy link
Member Author

Choose a reason for hiding this comment

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

Or you are planning to add more command line args like --limit-name-in-labels when the value of the telemtry is exhaustive??

Yes, or… let's say I did want that to be generic to not tie it to any implementation, as we'll probably iterate over this. Hence the enum as well, so we can add values. e.g. to support better tracing

@eguzki
Copy link
Contributor

eguzki commented Nov 9, 2023

Code wise it is correct

@alexsnaps alexsnaps marked this pull request as ready for review November 13, 2023 18:00
@alexsnaps alexsnaps requested review from a team and eguzki November 13, 2023 18:00
@alexsnaps
Copy link
Member Author

Rebased and ready for merge/releasing

@alexsnaps alexsnaps added this to the v0.7.0 milestone Nov 13, 2023
@alexsnaps alexsnaps merged commit 5c9f159 into main Nov 14, 2023
11 checks passed
@alexsnaps alexsnaps deleted the rlp-named-2 branch November 14, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: To test
Development

Successfully merging this pull request may close these issues.

3 participants