feat: warp10 exporter #76

bpetit · 2021-02-27T18:49:35Z

Work in progress for #22

uggla · 2021-03-02T10:34:24Z

That's sounds good to me so far. But I have only done a quick review.

As proposed in the rieman client MR:

@bpetit, if it is ok for you, as soon as this PR will be merged, I will open a new one to do the proposed changes to reduce duplication.
This may also simplify implementations of new exporters.
How do you want to proceed, would you like to discuss the idea upfront or would you like me to propose an example MR first.

I had a lack of time to propose something here. :(

Basically, we are doing almost the same stuffs in prom, riemann and warp10 exporters. This led to duplicated code and long methods so errors .
So I wonder if we should not make an effort to factorize code now because each time we will have a new exporter it will make the task harder and harder.

If it is possible for you, we could do a pair programming session to get rid of this issue. I think it will be the fastest way to progress on this.
What do you think about it ?

uggla · 2021-03-02T10:06:23Z

src/exporters/riemann.rs

-    let minor_version = version_parts.next().unwrap();
-    format!("{}.{}{}", major_version, patch_version, minor_version)
-}
+//  Copyright 2020 The scaphandre authors.


Oh great to add the licence !

uggla · 2021-03-02T10:37:54Z

src/exporters/warpten.rs

+            ExporterOption {
+                default_value: None,
+                help: String::from("Time step between measurements, in seconds."),
+                long: String::from("qemu"),


Hum I guess there is an issue in the option name, or more likely this option is not required.

I've not yet pushed the code attached to it, sorry :)
I'm actually using it in the exporter (and I'm currently trying to fix some issues related to the label resulting from it)

uggla · 2021-03-02T10:39:41Z

src/exporters/warpten.rs

+                takes_value: true,
+            },
+        );
+        options.insert(


Not really fan of this option, I would merge with the host one and create an url option maybe.

I've done it like that because another option to send data to warp10 is websockets. Because the uri for that would look like ws://host/api/v0...
I thought it would be cleaner to have a separate variable to manage the protocol. 🤔

Make sens in that case.

uggla · 2021-03-02T10:48:20Z

src/exporters/riemann.rs

                topo_procs_len,
            );
-
+            //TODO: metric sent twice ?


Yep looks like it is the case :)

uggla · 2021-03-02T10:49:56Z

src/exporters/riemann.rs

                    socket.stat_buffer.len(),
                );
-
+                //TODO: fix metric


Maybe provide a bit more details here.

Here the value is socket.stat_buffer.len() for both scaph_self_socket_stats_nb and scaph_self_socket_records_nb.

yep pitfall here. :(

uggla · 2021-03-02T10:52:53Z

src/exporters/utils.rs

    None
 }

+pub fn get_scaphandre_version() -> String {


uggla · 2021-03-02T10:55:35Z

src/exporters/warpten.rs

+                takes_value: true,
+            },
+        );
+        options.insert(


Hum i guess it is a secret. I think it is "safer" to pass it in a file and not cmdline.
We may allow to pass it from cmdline with a warning saying it is dangerous.

You're right, it's a very dumb way to do it. I've done it like that first to bootstrap the feature quickly. I'll fix that prior merging.

I think an env var and a file option could be nice. An env var will probably make it easier to integrate with docker, k8s secrets.

I've added the env var option, let's create the file option when someone asks for it explicitely :)

uggla · 2021-03-02T11:07:52Z

src/exporters/warpten.rs

+            None,
+            String::from("scaph_self_version"),
+            labels.clone(),
+            warp10::Value::Double(scaphandre_version.parse::<f64>().unwrap()),


Another thing we could factorize maybe. Maybe, we could leverage the Metric trait to send something whatever the type it will select the appropriate method (Double, Long, ...).

I think you have a point here. This would be much cleaner. I don't yet imagine how it would integrate with the warp10 lib however.

uggla · 2021-03-02T11:11:13Z

You should prefix the MR with WIP: this will avoid to accidentally merge it.

bpetit · 2021-03-03T11:09:16Z

That's sounds good to me so far. But I have only done a quick review.

Thanks a lot !

As proposed in the rieman client MR:
@bpetit, if it is ok for you, as soon as this PR will be merged, I will open a new one to do the proposed changes to reduce duplication.
This may also simplify implementations of new exporters.
How do you want to proceed, would you like to discuss the idea upfront or would you like me to propose an example MR first.
I had a lack of time to propose something here. :(

Basically, we are doing almost the same stuffs in prom, riemann and warp10 exporters. This led to duplicated code and long methods so errors .
So I wonder if we should not make an effort to factorize code now because each time we will have a new exporter it will make the task harder and harder.

I agree, it would be much cleaner.

If it is possible for you, we could do a pair programming session to get rid of this issue. I think it will be the fastest way to progress on this.
What do you think about it ?

Great idea, let's discuss about a meeting schedule that fits both of our agendas to work on that :)

uggla · 2021-03-05T09:00:44Z

docs_src/references/exporter-warp10.md

+
+## Metrics exposed
+
+Typically the Warp10 exporter is working in the same way as the riemann exporter regarding metrics. Please look at details in [Prometheus exporter](exporter-prometheus.md) documentations to get the extensive list of metrics available.


Typically the Warp10 exporter is working in the same way as the prometheus and riemann exporters regarding metrics.

…g/scaphandre into feature/fr-#22-warp10-exporter

feat: warp10 exporter with first base metrics sent

a22b0da

bpetit self-assigned this Feb 27, 2021

bpetit changed the title ~~feat: warp10 exporter with first base metrics sent~~ feat: warp10 exporter Feb 27, 2021

bpetit added 9 commits February 27, 2021 21:55

style: cargo fmt

b76a98e

feat: adding metrics to warp10 exporter

49ee747

feat: adding process metrics in warp10 exporter

243fd72

style: cargo fmt

1dc2b56

feat: adding qemu option in warp10 exporter

cc72f83

style: cargo fmt

85ec3d5

fix: update warp10 and time crates

1253836

style: cargo fmt

ad1b6cc

fix: cargo clippy

2be0ec0

bpetit requested a review from uggla March 1, 2021 18:21

bpetit linked an issue Mar 1, 2021 that may be closed by this pull request

Warp10 exporter #22

Closed

uggla reviewed Mar 2, 2021

View reviewed changes

bpetit changed the title ~~feat: warp10 exporter~~ WIP: feat: warp10 exporter Mar 3, 2021

bpetit added 5 commits March 3, 2021 18:06

fix: putting cmdline items in the right order

8e0677b

fix: removing unused split data feature

5f2adf5

fix: commenting unused import

aa3ea15

docs: cleaning useless comments and adding some description

4072cd0

docs: added doc for warp10 exporter

72b7a55

uggla reviewed Mar 5, 2021

View reviewed changes

bpetit added 5 commits March 10, 2021 19:30

chore: disabling read-token for now

53696f9

docs: fixing conflict

0abcb2f

feat: warp10 write-token as an env var

5571d0a

Merge branch 'main' into feature/fr-#22-warp10-exporter

c97c206

fix: enabling default value for step

4849854

bpetit added 5 commits March 11, 2021 11:32

docs: adding write-token to doc

9bf4001

Merge branch 'feature/fr-#22-warp10-exporter' of github.com:hubblo-or…

26f8703

…g/scaphandre into feature/fr-#22-warp10-exporter

docs: fixing weird sentence

a1ddfac

fix: metrics with wrong data in riemann exporter

2cd4d30

docs: updating docs

e59fac3

bpetit merged commit f47f332 into main Mar 11, 2021

bpetit changed the title ~~WIP: feat: warp10 exporter~~ feat: warp10 exporter Mar 11, 2021


		## Metrics exposed

		Typically the Warp10 exporter is working in the same way as the riemann exporter regarding metrics. Please look at details in [Prometheus exporter](exporter-prometheus.md) documentations to get the extensive list of metrics available. No newline at end of file

Uh oh!

feat: warp10 exporter #76

feat: warp10 exporter #76

Uh oh!

Conversation

bpetit commented Feb 27, 2021

Uh oh!

uggla commented Mar 2, 2021

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!

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!

uggla commented Mar 2, 2021

Uh oh!

bpetit commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants