Add example for actix-web framework #54

oriontvv · 2022-03-18T15:15:40Z

Resolves #53

mxinden

Couple of comments. Thanks for the work here. More examples are very much appreciated.

mxinden · 2022-03-21T10:01:12Z

examples/actix-web.rs

+
+/// Holds all metrics.
+/// We shouldn't store metrics inside a MetricsCollector -
+/// overwise we would have to take a lock for each increment of any metric


Suggested change

/// overwise we would have to take a lock for each increment of any metric

/// otherwise we would have to take a lock for each increment of any metric.

mxinden · 2022-03-21T10:02:57Z

examples/actix-web.rs

+use prometheus_client::registry::Registry;
+
+#[derive(Clone, Hash, PartialEq, Eq, Encode)]
+pub enum RequestType {


Suggested change

pub enum RequestType {

pub enum Method {

In the tide example we call it Method. I don't really care about the names, though I would prefer to be consistent. What do you think?

https://github.com/prometheus/client_rust/blob/master/examples/tide.rs

Good point, agree.

mxinden · 2022-03-21T10:12:00Z

examples/actix-web.rs

+async fn main() -> std::io::Result<()> {
+    let metrics = web::Data::new(Metrics::new());
+    // We have to wrap it with Mutex because `Registry` doesn't implement Copy-trait
+    let metrics_collector = web::Data::new(Mutex::new(MetricsCollector::new(&metrics)));


Note that encode takes an immutable reference to Registry. Thus there is no need for a Mutex in case you never register new metrics after the first Prometheus scrape. An Arc should suffice.

Reasonable, please check out a comment

mxinden · 2022-03-21T10:15:14Z

examples/actix-web.rs

+}
+
+/// Registers and collects metrics
+pub struct MetricsCollector {


Why this wrapper around Registry? Despite the fact that client_rust does use the Collector abstraction (yet see #49), if I am not mistaken this concept is not wrapping a Registry in the other implementations, but rather the other way around.

I tried to avoid Mutex here in this abstraction, but didn't found the one.. would like to see any suggestions

With #57 fixed, is this still needed @oriontvv?

Thanks, it looks like the PR fixes this issue, will try to adapt the example in few days.

mxinden · 2022-07-18T11:28:32Z

👋 @oriontvv still interested in finishing this pull request?

oriontvv · 2022-07-18T12:23:23Z

Yes, I will simplify the example without unnecessary wrapper in few days 👋

mxinden

Very clean. Two more comments. Otherwise this looks good to me.

mxinden · 2022-07-22T09:14:28Z

CHANGELOG.md

+### Added
+
+- Example for actix-web framework. See [PR 54].
+


Suggested change

[PR 54]: https://github.com/prometheus/client_rust/pull/54/

mxinden · 2022-07-22T09:16:02Z

examples/actix-web.rs

+    let mut buf = Vec::new();
+    encode(&mut buf, &state.registry)?;
+    let body = std::str::from_utf8(buf.as_slice()).unwrap().to_string();
+    Ok(HttpResponse::Ok().body(body))


Would one not need to set the content type?

client_rust/examples/tide.rs

Line 37 in 6b7a345

.content_type("application/openmetrics-text; version=1.0.0; charset=utf-8")

Actually no, my experience shows that it is not required. Everything works well without this detail.

According to the OpenMetrics specification it "MUST" be set. Am I missing something @oriontvv?

https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure

//CC @RichiH

Okay, I'll add this. (btw, we use at work without it and everything is fine)

examples/actix-web.rs

Signed-off-by: oriontvv <taranov.vv@gmail.com>

There is no need for mutable access to the `Registry` once all metrics have been registered. Encoding the registered metrics does not require a mutable reference. With that in mind, an `Arc` is good enough. There is no need for a `Mutex`. Signed-off-by: Max Inden <mail@max-inden.de> Signed-off-by: oriontvv <taranov.vv@gmail.com>

This will allow the default `Registry` to be used in multi-threaded environments (e.g. web servers) where a ref to the registry needs to be shared between threads. Closes #57. Signed-off-by: Ben Sully <ben.sully@grafana.com> Signed-off-by: Max Inden <mail@max-inden.de> Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: oriontvv <taranov.vv@gmail.com>

Signed-off-by: oriontvv <taranov.vv@gmail.com>

oriontvv · 2022-07-22T22:09:00Z

Sorry for the garbage commits. I'm trying to figure out how to deal with your tricky checks of signing.

mxinden · 2022-07-25T15:26:33Z

I'm trying to figure out how to deal with your tricky checks of signing.

Sorry for the trouble. It is unfortunate that we require this from a legal side.

mxinden · 2022-07-25T15:27:12Z

src/registry.rs

@@ -57,7 +57,6 @@ use std::borrow::Cow;
 /// #                "# EOF\n";
 /// # assert_eq!(expected, String::from_utf8(buffer).unwrap());
 /// ```
-#[derive(Debug)]


Why this change?

Sorry, it was because of unsuccessful conflicts with rebase, fixed

Signed-off-by: oriontvv <taranov.vv@gmail.com>

mxinden

Looks good to me. Just needs the sign-off. Feel free to just squash into a single signed off commit in case that is more convenient for you @oriontvv.

Signed-off-by: oriontvv <taranov.vv@gmail.com>

oriontvv · 2022-07-28T09:40:13Z

reopened in #76

mxinden reviewed Mar 21, 2022

View reviewed changes

mxinden reviewed Jul 22, 2022

View reviewed changes

oriontvv and others added 8 commits July 23, 2022 00:54

Add example for actix-web

e19b5a2

Signed-off-by: oriontvv <taranov.vv@gmail.com>

fix typo

071e5a6

Signed-off-by: oriontvv <taranov.vv@gmail.com>

Update changelog

304b9e1

Signed-off-by: oriontvv <taranov.vv@gmail.com>

Consistent naming for examples

8d338b2

Signed-off-by: oriontvv <taranov.vv@gmail.com>

simplify example, add actix-web to dev deps

d9eadbb

Signed-off-by: oriontvv <taranov.vv@gmail.com>

Fix link

bf22b8b

Signed-off-by: oriontvv <taranov.vv@gmail.com>

mxinden reviewed Jul 25, 2022

View reviewed changes

oriontvv added 5 commits July 25, 2022 21:45

revert redundant change

891bc51

Signed-off-by: oriontvv <taranov.vv@gmail.com>

revert mess commits because of rebase

e561d02

add header for openmetrics

0156203

revert mess change

fd422df

Merge branch 'master' into master

a0c7f67

mxinden approved these changes Jul 27, 2022

View reviewed changes

fix clippy

9ad9e7a

Signed-off-by: oriontvv <taranov.vv@gmail.com>

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add example for actix-web framework #54

Add example for actix-web framework #54

oriontvv commented Mar 18, 2022

mxinden left a comment

mxinden Mar 21, 2022

mxinden Mar 21, 2022

oriontvv Mar 26, 2022

mxinden Mar 21, 2022

oriontvv Mar 26, 2022

mxinden Mar 21, 2022

oriontvv Mar 26, 2022

mxinden Apr 8, 2022

mxinden Apr 8, 2022

oriontvv Apr 10, 2022

mxinden commented Jul 18, 2022

oriontvv commented Jul 18, 2022

mxinden left a comment

mxinden Jul 22, 2022

mxinden Jul 22, 2022

oriontvv Jul 22, 2022

mxinden Jul 25, 2022

mxinden Jul 25, 2022

oriontvv Jul 25, 2022 •

edited

Loading

oriontvv commented Jul 22, 2022

mxinden commented Jul 25, 2022

mxinden Jul 25, 2022

oriontvv Jul 25, 2022

mxinden left a comment

oriontvv commented Jul 28, 2022

	/// overwise we would have to take a lock for each increment of any metric
	/// otherwise we would have to take a lock for each increment of any metric.

Add example for actix-web framework #54

Add example for actix-web framework #54

Conversation

oriontvv commented Mar 18, 2022

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Jul 18, 2022

oriontvv commented Jul 18, 2022

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oriontvv Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

oriontvv commented Jul 22, 2022

mxinden commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

oriontvv commented Jul 28, 2022

oriontvv Jul 25, 2022 •

edited

Loading