Limits Header extension: add support for metric hierarchies with more than 2 levels by davidor · Pull Request #122 · 3scale/apisonator

davidor · 2019-09-03T10:54:19Z

This is part of #114 . It adapts the limits header extension to work with metric hierarchies of more than 2 levels.

davidor · 2019-09-03T14:33:14Z

+    assert_true metrics.each_with_index.all? do |metric, idx|
+      ascendants = Metric.ascendants(service_id, metric.name)
+
+      if idx == 0


This can be simplified using take(idx).

yes, are you changing it?

unleashed · 2019-09-03T14:27:51Z

          end
        end

+        # Returns the "ascendants" of a metric, that is, its parents,


can a metric have multiple parents?

yep, this was meant to correct the parents to parent, grandparent, ...

Right. The description is not accurate 👍

unleashed · 2019-09-03T14:30:10Z

+
+          parents_of_metric.reduce(parents_of_metric) do |acc, parent|
+            acc + ascendants(service_id, parent)
+          end


Have you considered using parents_of_metric.each and pushing to it as you go for a more readable and performant version avoiding recursion?

Not sure I follow. Can you expand a bit?

déjà vu https://github.com/3scale/apisonator/pull/119/files#r318661075

This is the exact same code as descendants - have you checked out how to DRY this up?

I haven't come up with anything that makes the code more readable.

This makes me feel uneasy, but oh well.

unleashed · 2019-09-03T14:37:25Z

+    assert_true metrics.each_with_index.all? do |metric, idx|
+      ascendants = Metric.ascendants(service_id, metric.name)
+
+      if idx == 0


yes, are you changing it?

… more flexible - Set usage limits optionally, so the caller can set others. - Add the plan ID to the hash returned

…vels

davidor · 2019-09-03T15:36:47Z

bors r=@unleashed

122: Limits Header extension: add support for metric hierarchies with more than 2 levels r=unleashed a=davidor This is part of #114 . It adapts the limits header extension to work with metric hierarchies of more than 2 levels. Co-authored-by: David Ortiz <z.david.ortiz@gmail.com>

bors · 2019-09-03T15:39:31Z

Build succeeded

ci/circleci

davidor force-pushed the limits-header-ext-with-multi-level-hierarchies branch from 5b282e6 to 3649a88 Compare September 3, 2019 14:19

davidor marked this pull request as ready for review September 3, 2019 14:21

davidor requested a review from unleashed September 3, 2019 14:21

davidor commented Sep 3, 2019

View reviewed changes

unleashed reviewed Sep 3, 2019

View reviewed changes

davidor force-pushed the limits-header-ext-with-multi-level-hierarchies branch from 3649a88 to fc15a4d Compare September 3, 2019 14:52

davidor added 8 commits September 3, 2019 16:59

metric: add #ascendants

dbba6fa

test/unit/metric: test #ascendants

9f78f15

usage_report: compute usage for all descendants

8a00112

status: take into account the whole hierarchy in #limits_headers

eac6cfa

test/test_helpers/fixtures: make #setup_service_with_metric_hierarchy…

6ca20aa

… more flexible - Set usage limits optionally, so the caller can set others. - Add the plan ID to the hash returned

test/integration/authorize/limit_headers: test hierarchies with >2 le…

892f41d

…vels

test/integration/authrep/limit_headers: test hierarchies with >2 levels

b5cd2b9

test/integration/oauth/limit_headers: test hierarchies with >2 levels

446ab36

davidor force-pushed the limits-header-ext-with-multi-level-hierarchies branch from fc15a4d to 446ab36 Compare September 3, 2019 14:59

unleashed approved these changes Sep 3, 2019

View reviewed changes

bors Bot merged commit 446ab36 into master Sep 3, 2019

bors Bot deleted the limits-header-ext-with-multi-level-hierarchies branch September 3, 2019 15:39

davidor mentioned this pull request Sep 3, 2019

Ensure that metric hierarchies of more than 2 levels are supported #114

Closed

Conversation

davidor commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidor Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidor Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidor commented Sep 3, 2019

Uh oh!

bors Bot commented Sep 3, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davidor commented Sep 3, 2019 •

edited

Loading

davidor Sep 3, 2019 •

edited

Loading

davidor Sep 3, 2019 •

edited

Loading