cargo: update prometheus to 0.7 #125

lucab · 2019-06-20T17:12:41Z

This updates prometheus library to latest version.

steveej

I'd feel more comfortable stamping this if we had metrics tests. Do you have time to work on that within the scope of this PR?

steveej · 2019-06-20T17:29:07Z

quay/Cargo.toml

@@ -18,4 +18,4 @@ tokio = "^0.1.14"

 [features]
 test-net = []
-test-net-private = []


this change seems unrelated

lucab · 2019-06-20T17:54:45Z

@steveej I think so, but a complete metrics test would require an integration suite. Would a "the metrics handler returns 200 and a non-empty body" test be enough here (maybe with some keyword grepping too)?

steveej · 2019-06-20T19:04:25Z

@lucab

Would a "the metrics handler returns 200 and a non-empty body" test be enough here

I'm fine with that for this PR 👍

lucab · 2019-06-21T09:24:48Z

@steveej this now comes with tests too, PTAL.

steveej

I haven't fully gone through here but my general suggestion for all tests is that we use Result like in regular code.

graph-builder/src/status.rs

lucab · 2019-06-21T11:28:35Z

@steveej I've updated to use Fallible everywhere in those tests, does it look better now?

steveej

except for the duplicate common_init* method this looks pretty good, thanks for the tests!

* I went back and forth two twice and they looked the same

policy-engine/src/metrics.rs

steveej

Since you're using failure in here you can use the ensure!() macro here and elsewhere instead of assert!().

steveej · 2019-06-24T11:39:53Z

graph-builder/src/status.rs

+        assert!(resp.body().is_binary());
+
+        if let actix_web::Body::Binary(body) = resp.body() {
+            assert!(!body.is_empty());


Since you're using failure in here you can use the ensure!() macro here and elsewhere instead of assert!()

assert and ensure are not directly interchangeable, as the latter doesn't have fileline information and additional introspection details provided by the former.

I'm sticking to assert for enforcing invariants, but feel free to have a spike ticket to investigate/build a blended ensure+assert_* macro.

This updates prometheus library to latest version.

This adds a simple test for the metrics handler.

lucab · 2019-06-24T14:43:32Z

I've moved shared testing helpers to commons::testing, but kept the assert checks. PTAL.

steveej · 2019-06-24T18:19:02Z

I've moved shared testing helpers to commons::testing, but kept the assert checks. PTAL.

Thanks for walking the extra mile! 🏃‍♂️

/lgtm

openshift-ci-robot · 2019-06-24T18:19:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lucab, steveeJ

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lucab,steveeJ]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

In preparation for consuming the 'version' file that graph-data grew in [1]. This gets us the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

Consuming the 'version' file that graph-data grew in [1], so the logs will contain more useful error messages when the plugin is fed an incompatible schema version. Bumping the graph-data commit used for CI moves us to the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

In preparation for consuming the 'version' file that graph-data grew in [1]. This gets us the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

Consuming the 'version' file that graph-data grew in [1], so the logs will contain more useful error messages when the plugin is fed an incompatible schema version. Bumping the graph-data commit used for CI moves us to the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

In preparation for consuming the 'version' file that graph-data grew in [1]. This gets us the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

Consuming the 'version' file that graph-data grew in [1], so the logs will contain more useful error messages when the plugin is fed an incompatible schema version. Bumping the graph-data commit used for CI moves us to the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

openshift-ci-robot requested a review from steveej June 20, 2019 17:12

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 20, 2019

steveej reviewed Jun 20, 2019

View reviewed changes

cargo: add twoway (for tests)

3ff4e4b

lucab force-pushed the ups/cargo-prom branch from b1b8a3d to 57312c6 Compare June 21, 2019 09:22

steveej reviewed Jun 21, 2019

View reviewed changes

lucab force-pushed the ups/cargo-prom branch from 57312c6 to 43cbccb Compare June 21, 2019 11:27

steveej reviewed Jun 21, 2019

View reviewed changes

policy-engine/src/metrics.rs Outdated Show resolved Hide resolved

steveej reviewed Jun 24, 2019

View reviewed changes

lucab added 7 commits June 24, 2019 14:13

cargo/commons: move to 2018 edition

7e0f612

cargo/commons: add tokio and env_logger

0ae4373

cargo: update prometheus to 0.7

ab3e6d2

This updates prometheus library to latest version.

lockfile: refresh after cargo changes

c621088

commons: add testing helpers

6640bc0

policy-engine: add basic metrics test

4f1eb0a

This adds a simple test for the metrics handler.

graph-buider: add basic metrics test

0c7d9bc

This adds a simple test for the metrics handler.

lucab force-pushed the ups/cargo-prom branch from 43cbccb to 0c7d9bc Compare June 24, 2019 14:25

openshift-ci-robot assigned steveej Jun 24, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019

openshift-merge-robot merged commit 4dad264 into openshift:master Jun 24, 2019

lucab deleted the ups/cargo-prom branch June 24, 2019 19:28

steveej mentioned this pull request Jun 25, 2019

*: bump actix/actix-web to 0.8.3/1.0.2 #123

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo: update prometheus to 0.7 #125

cargo: update prometheus to 0.7 #125

lucab commented Jun 20, 2019

steveej left a comment

steveej Jun 20, 2019

lucab commented Jun 20, 2019

steveej commented Jun 20, 2019

lucab commented Jun 21, 2019

steveej left a comment

lucab commented Jun 21, 2019

steveej left a comment

steveej left a comment

steveej Jun 24, 2019

lucab Jun 24, 2019 •

edited

Loading

lucab commented Jun 24, 2019 •

edited

Loading

steveej commented Jun 24, 2019

openshift-ci-robot commented Jun 24, 2019

cargo: update prometheus to 0.7 #125

cargo: update prometheus to 0.7 #125

Conversation

lucab commented Jun 20, 2019

steveej left a comment

Choose a reason for hiding this comment

steveej Jun 20, 2019

Choose a reason for hiding this comment

lucab commented Jun 20, 2019

steveej commented Jun 20, 2019

lucab commented Jun 21, 2019

steveej left a comment

Choose a reason for hiding this comment

lucab commented Jun 21, 2019

steveej left a comment

Choose a reason for hiding this comment

steveej left a comment

Choose a reason for hiding this comment

steveej Jun 24, 2019

Choose a reason for hiding this comment

lucab Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

lucab commented Jun 24, 2019 • edited Loading

steveej commented Jun 24, 2019

openshift-ci-robot commented Jun 24, 2019

lucab Jun 24, 2019 •

edited

Loading

lucab commented Jun 24, 2019 •

edited

Loading