Ping metrics #23

dmke · 2022-04-01T15:27:32Z

This enables fetching device statistics from UNMS, extracting the ping RTT values from it, and export them back to Prometheus.

mraerino · 2022-04-12T10:15:48Z

the diff looks like it contains things from #22 - maybe rebase just in this case?

mraerino

the implementation looks good to me.

could you make this feature available behind a flag or query parameter though?
i'm not sure all users of this service want to cause these additional requests to the API.

dmke · 2022-04-12T12:58:32Z

Sure, you got some name in mind? Previous iterations used --with-statistics and later --enable-ping-metrics.

The former name stemmed from the API endpoint (/devices/{id}/statistics), but that didn't feel right, because we're only grabbing the ping metrics. The latter does reflect more the actual purpose, but will become outdated should any of the other metrics become interesting (among others, the API provides data for client connection quality, uplink/downlink/wireless utilization, WiFi signal strength, temperature).

Maybe something akin to --extra-metrics=<comma separated list> (i.e. --extra-metrics=ping), to be safe for future extension?

This is in preparation to fetch ping statistics from UNMS. The endpoint returns A LOT more data, but for now this contains only the minimal set.

This enables fetching device statistics from UNMS, extracting the ping RTT values from it, and export them back to prometheus. Since fetching statistics requires talking to another, device-specific endpoint, the context timeout is increased from 5 to 30s. A future change should pass the request context from the promhttp endpoint down to the fetchDeviceData() method, to make the timout depend on Prometheus' scrape request life cycle.

mraerino · 2022-04-14T13:50:28Z

But I'm not a fan of underscores in flags. One option could be to just use a single-word flag name (--extras). Any other suggestions?

you can use StringSliceVar and pass a pointer to the config field as the first argument. that should work in any case

mraerino · 2022-04-14T14:50:23Z

wait, i have a feeling you're confusing some things here:

reading flags and config file is done via viper. you manually create the flags and use the mapstructure tag to read from the config and associate flags
env var handling is done via envconfig and you use the envconfig tag to specify the name

so setting envconfig:"extra_metrics" should solve the case where the env var does not work

dmke · 2022-04-14T15:18:22Z

[...] you're confusing [...] mapstructure [and] envconfig

You're right, I tripped over that.

setting envconfig:"extra_metrics" should solve the case

Already done here. That's not it.

I'd probably need to set mapstructure:"extra-metrics", but that would then introduce an inconsistency with mapstructure:"log_level"...

I've settled on a single config word (extras), which may not be 100% perfect, but avoids these edge cases.

mraerino · 2022-04-16T20:11:26Z

@dmke i opened a PR against your PR to get the dash option working: digineo#1

the tests made it a lot easier to implement, thanks for that!

mraerino · 2022-04-17T12:56:17Z

if you want to resolve the conflicts, this might be good to go!

dmke · 2022-04-17T14:17:45Z

Done.

I had to re-introduce the error-return for internal/handler.New(), I knew it was there for something important :)

mraerino

one thing

mraerino · 2022-04-17T14:44:47Z

exporter/exporter.go

@@ -241,6 +247,21 @@ func (e *Exporter) collectImpl(ctx context.Context, out chan<- prom.Metric) erro
 			out <- e.newMetric("wan_rx_rate", prom.GaugeValue, wanIF.Statistics.Rxrate, deviceLabels...)
 			out <- e.newMetric("wan_tx_rate", prom.GaugeValue, wanIF.Statistics.Txrate, deviceLabels...)
 		}
+
+		// Ping metrics, if enabled
+		if e.extras.Ping {


do you need to check this here? should be fine to use the nil-check in PingMetrics right? that way the fetchDeviceData is the only place it's handled, which i'd like.

I'd rather leave it as is, because future extensions to the models.DeviceStatistics struct (hint: coming very soon) will require this conditional where it is now.

mraerino

interesting feature. thanks for the contribution!

dmke mentioned this pull request Apr 5, 2022

Cleanup Exporter a bit #22

Merged

dmke force-pushed the feat/ping-metrics branch from 754a49b to b58df3e Compare April 5, 2022 11:48

This comment was marked as outdated.

Sign in to view

dmke force-pushed the feat/ping-metrics branch 7 times, most recently from 8dfe88a to ce1767e Compare April 9, 2022 19:18

dmke mentioned this pull request Apr 9, 2022

Proposal: Improve metrics collector situation #28

Closed

dmke force-pushed the feat/ping-metrics branch from ce1767e to 7d9d9d4 Compare April 11, 2022 19:29

This comment was marked as outdated.

Sign in to view

mraerino reviewed Apr 12, 2022

View reviewed changes

dmke added 2 commits April 12, 2022 17:31

Add /devices/{id}/statistics path to Swagger config

6322a5d

This is in preparation to fetch ping statistics from UNMS. The endpoint returns A LOT more data, but for now this contains only the minimal set.

dmke force-pushed the feat/ping-metrics branch from 661f4f0 to 726cbf9 Compare April 12, 2022 15:32

This comment was marked as outdated.

Sign in to view

dmke added 3 commits April 14, 2022 17:10

Ping metrics: disable by default, add config knob

18a4d69

Ping metrics: update README

dbf8e04

Make examples in README consistent

ca368e7

dmke added 3 commits April 14, 2022 17:20

Merge branch 'main' into feat/ping-metrics

7317917

Ping metrics: cleanup dead code

2404735

Merge branch 'main' into feat/ping-metrics

81c075f

dmke and others added 3 commits April 15, 2022 15:03

Merge branch 'main' into feat/ping-metrics

d5d1952

Ping metrics: add tests for env vars/CLI flags

f9f2cda

Allow usage of dash in metrics option

c51eeba

Update docs and extend test case

310826c

Merge branch 'main' into feat/ping-metrics

d3e81b1

mraerino reviewed Apr 17, 2022

View reviewed changes

mraerino merged commit 1ac4c3f into ffddorf:main Apr 17, 2022

dmke deleted the feat/ping-metrics branch April 17, 2022 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ping metrics #23

Ping metrics #23

dmke commented Apr 1, 2022 •

edited

Loading

This comment was marked as outdated.

mraerino commented Apr 12, 2022

This comment was marked as outdated.

mraerino left a comment

dmke commented Apr 12, 2022

This comment was marked as outdated.

mraerino commented Apr 14, 2022 •

edited

Loading

This comment was marked as outdated.

mraerino commented Apr 14, 2022 •

edited

Loading

dmke commented Apr 14, 2022

mraerino commented Apr 16, 2022

mraerino commented Apr 17, 2022

dmke commented Apr 17, 2022

mraerino left a comment

mraerino Apr 17, 2022

dmke Apr 17, 2022

mraerino left a comment

Ping metrics #23

Ping metrics #23

Conversation

dmke commented Apr 1, 2022 • edited Loading

This comment was marked as outdated.

mraerino commented Apr 12, 2022

This comment was marked as outdated.

mraerino left a comment

Choose a reason for hiding this comment

dmke commented Apr 12, 2022

This comment was marked as outdated.

mraerino commented Apr 14, 2022 • edited Loading

This comment was marked as outdated.

mraerino commented Apr 14, 2022 • edited Loading

dmke commented Apr 14, 2022

mraerino commented Apr 16, 2022

mraerino commented Apr 17, 2022

dmke commented Apr 17, 2022

mraerino left a comment

Choose a reason for hiding this comment

mraerino Apr 17, 2022

Choose a reason for hiding this comment

dmke Apr 17, 2022

Choose a reason for hiding this comment

mraerino left a comment

Choose a reason for hiding this comment

dmke commented Apr 1, 2022 •

edited

Loading

mraerino commented Apr 14, 2022 •

edited

Loading

mraerino commented Apr 14, 2022 •

edited

Loading