Adaptive Load metrics evaluator library by eric846 · Pull Request #495 · envoyproxy/nighthawk

eric846 · 2020-08-27T19:45:03Z

A library that combines the latest Nighthawk Service response, metric and threshold spec configuration, and results from MetricsPlugins to produce metric scores.

Part 5 of splitting PR #483.

update from master

merge from upstream

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

…double Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

…variable_setter Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

merge from upstream

…ent.Output turns out not to include the status Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

…plugin names, log thresholds only once per session Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

wjuan-AFK · 2020-08-28T15:47:29Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+   */
+  virtual void
+  ExtractMetricSpecs(const nighthawk::adaptive_load::AdaptiveLoadSessionSpec& spec,
+                     std::vector<const nighthawk::adaptive_load::MetricSpec*>& metric_specs,


Alternatively, could we not just return the map, and retrieve the keys specifically if we need that? I also don't like having two separate data structures that are supposed to be in sync with each other implicitly.

wjuan-AFK · 2020-08-28T16:01:34Z

test/adaptive_load/metrics_evaluator_test.cc

+  return response;
+}
+
+TEST(EvaluateMetric, SetsMetricId) {


I think we prefer the ConditionAWillResultinB format for naming these.

Added some more detail throughout.

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf

looks great to me, one last question regarding the return type of ExtractMetricSpecs

oschaaf · 2020-09-04T19:44:11Z

source/adaptive_load/metrics_evaluator_impl.cc

+      metric_spec_list_map.second;
+
+  std::vector<std::string> errors;
+  for (const MetricSpec* metric_spec : metric_specs) {


Reading this, I'm still wondering; it seems to me that having ExtractMetricSpecs return a vector<std::pair<MetricSpec*, ThresholdSpec*>> could simplify that interface as well as the code here; wouldn't that avoid the map lookups? The map usage would be in implementation detail in ExtractMetricSpecs, and not propagate outside of it. Of course, that might thwart re-use for other purposes perhaps.

Oh that's a good idea, it preserves the order and is simpler. I definitely don't need the map. If someone did need a map later, I think they could pass the vector of pairs into a map constructor.

Updated now to return a vector of pairs.

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf

LGTM

wjuan-AFK · 2020-09-08T17:44:03Z

LGTM

dubious90 · 2020-09-09T15:16:19Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+namespace Nighthawk {
+
+/**
+ * An interface with utilities for translating between metrics definitions, thresholds, scores,


Can we add a usage example here? I have other confusions about the API here, but if you feel like your example will resolve them, feel free to ignore other points.

I added a better explanation of what this library does. Only one of the methods is expected to be called directly.

dubious90 · 2020-09-09T15:27:21Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+   * it according to a threshold and scoring function.
+   *
+   * @param metric_spec The metric spec identifying the metric by name and plugin name.
+   * @param metrics_plugin An already activated MetricsPlugin used by the metric_spec.


The word activated here is confusing to me. I'm not sure it means: Two questions:

Would someone using this file usually already know what this means, making this less of a concern?

Is the fact that the plugin would already need to be "activated" more or less obvious if you know what it means, making this over-specified?

Dropped that wording, it should be clearer now.

dubious90 · 2020-09-09T15:28:41Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+
+  /**
+   * Given a MetricSpec, obtains a single metric value from the MetricPlugin and optionally scores
+   * it according to a threshold and scoring function.


I think this would be easier to follow if we said according to a ThresholdSpec. Then one would look for the threshold spec in the parameters, and see that it is a threshold with a scoring function.

dubious90 · 2020-09-09T15:36:09Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+namespace Nighthawk {
+
+/**
+ * An interface with utilities for translating between metrics definitions, thresholds, scores,


This comment seems to be focused on what it works with, rather than what it actually does? It's possible this will be obvious after the usage is included, but can we try to clarify its purpose if not

This should be clearer now.

include/nighthawk/adaptive_load/metrics_evaluator.h

dubious90 · 2020-09-09T15:52:03Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+
+  /**
+   * Analyzes a Nighthawk Service benchmark against configured MetricThresholds. Queries
+   * outside MetricsPlugins for current metric values, and/or uses "nighthawk.builtin" plugin to


These feel like implementation details to me.

Is nighthawk.builtin a MetricsPlugin? If so, I don't think this sentence ("Queries outside MetricsPlugins") is necessary. The documentation for nighthawk.builtin should rest within that plugin itself, and this would imply we only have one internal MetricsPlugin, which I don't think is a good assumption to make for the future.

Dropping this discussion of the builtin plugin. Mentioned the nighthawk.builtin in some of the parameter descriptions.

dubious90 · 2020-09-09T15:59:24Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+   * Analyzes a Nighthawk Service benchmark against configured MetricThresholds. Queries
+   * outside MetricsPlugins for current metric values, and/or uses "nighthawk.builtin" plugin to
+   * extract stats and counters from the latest Nighthawk Service output. The Nighthawk benchmark is
+   * assumed to have finished recently so values from MetricsPlugins will be relevant.


If I understand this, I think it might be better phrased as the following. (Please feel free to edit or push back):
Assumes that the values from MetricsPlugins correspond timewise with the nighthawk benchmark.

dubious90 · 2020-09-09T16:05:11Z

include/nighthawk/adaptive_load/metrics_evaluator.h

+   * @param nighthawk_response Proto returned from Nighthawk Service describing the latest single
+   * benchmark session.
+   * @param spec Top-level proto defining the adaptive load session.
+   * @param name_to_custom_metrics_plugin_map Common map from plugin names to MetricsPlugins, loaded


the second clause of this is describing what the caller does, right? I don't think we should be documenting the calling code from this function. (What if it's called by something else in the future, especially since this is an interface, rather than the impl file)

If there are constraints around this, it might be better to describe it from this function's perspective:
"Should not change between calls within one adaptive load session."

But if that's a constraint here, that sounds like an architectural oddity I'd have questions about.

I see, dropped the information about how the caller operates and and added a description of the true constraints.

source/adaptive_load/metrics_evaluator_impl.cc

test/adaptive_load/metrics_evaluator_test.cc

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 added 30 commits June 1, 2020 17:23

Merge pull request #5 from envoyproxy/master

8ea442d

update from master

Merge pull request #6 from envoyproxy/master

5ac755a

merge from upstream

Merge pull request #7 from envoyproxy/master

b8c25a5

merge from upstream

initial commit

1c19c68

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix comments

7050686

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix format

0776563

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

rename adaptive_rps to adaptive_load

16fd8f6

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add field_selector in example

c383010

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix example comment

6e1a483

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix format

4ef1140

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add support for fault injection headers

4111bf4

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

replace linear and binary search with exponential search

871a959

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add InputVariableSetter mechanism

1fd77c1

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add input variable setter to build file

edc36b2

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix syntax errors

4d0364e

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

rename samples/adaptive_rps

aed6d94

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

improve comments, change step controller initial value from int64 to …

d9ae87d

…double Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add proto validation rules, fix comments, make rps the default input_…

a05a6f5

…variable_setter Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix comment wording

8cd4d21

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

simplify protos, add defaults, specify required or optional

d814a96

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add missing newline

5f5a885

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Kick CI

7e20a78

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

simplify protos

9048267

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix format

306c0ec

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix some optional field comments and rules

d33f543

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge pull request #10 from envoyproxy/master

442cca9

merge from upstream

add Nighthawk status field in BenchmarkResult as nested nighthawk.cli…

677b783

…ent.Output turns out not to include the status Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

switch to standard Envoy plugin config proto, add prefix to internal …

cefb366

…plugin names, log thresholds only once per session Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into adaptive-rps-protos2

f3684df

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

create headers

5463051

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

wjuan-AFK reviewed Aug 28, 2020

View reviewed changes

eric846 added 8 commits August 29, 2020 15:18

change ExtractMetricSpecs output parameters to returned pair

ea9f562

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into master2

70705e9

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into master2

e576bc1

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into master2

1fca528

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

rename unit tests, fix compile error

f663975

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

make ExtractMetricSpecs return const containers

f367120

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge branch 'master2' into adaptive-rps-metric-evaluation

93f42dc

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

remove bazelrc

d2e502f

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 3, 2020

Merge remote-tracking branch 'upstream/master' into master2

ed32856

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf reviewed Sep 4, 2020

View reviewed changes

eric846 added 3 commits September 8, 2020 12:11

Merge remote-tracking branch 'upstream/master' into master2

eecf00d

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge branch 'master2' into adaptive-rps-metric-evaluation

788fa07

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

return vector of pairs from ExtractMetricSpecs

34b81da

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf previously approved these changes Sep 8, 2020

View reviewed changes

wjuan-AFK assigned dubious90 Sep 8, 2020

wjuan-AFK previously approved these changes Sep 8, 2020

View reviewed changes

dubious90 suggested changes Sep 9, 2020

View reviewed changes

dubious90 added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 9, 2020

improve comments

edc82db

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 dismissed stale reviews from wjuan-AFK and oschaaf via edc82db September 9, 2020 22:55

eric846 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 9, 2020

dubious90 approved these changes Sep 10, 2020

View reviewed changes

dubious90 merged commit 5fd7305 into envoyproxy:master Sep 10, 2020

Conversation

eric846 commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

oschaaf left a comment

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!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

wjuan-AFK commented Sep 8, 2020

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!

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eric846 commented Aug 27, 2020 •

edited

Loading