CON-181 - joaquin - Add PrometheusClient to ContentNode #2888

joaquincasares · 2022-04-08T23:02:50Z

Description

Add PrometheusClient to ContentNode with a simple test metric to graph track.js:upload() times.

Tests

Tested by uploading a track to a local audius-client w/ a remote-dev backend. This populated some tracks sample metrics while also testing the importer.

$ curl http://localhost:4001/prometheus_metrics
...
# HELP audius_cn_routes_tracks_runtime_seconds Runtimes for src.routes.tracks:/tracks
# TYPE audius_cn_routes_tracks_runtime_seconds histogram
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.005",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.01",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.025",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.05",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.1",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.25",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="0.5",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="1",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="2.5",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="5",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="10",scope="full"} 0
audius_cn_routes_tracks_runtime_seconds_bucket{le="+Inf",scope="full"} 1
audius_cn_routes_tracks_runtime_seconds_sum{scope="full"} 598
audius_cn_routes_tracks_runtime_seconds_count{scope="full"} 1

How will this change be monitored? Are there sufficient logs?

Once deployed to staging, I'll upload a track and ensure metrics appear.

Once soaking in prod, I'll ensure metrics appear without issues.

This reverts commit 6fa21a4.

SidSethi

i don't think i should be solo/primary reviewer here as i have zero context into prometheus. should be @dmanjunath

also would be good to clarify the purpose of these metrics vs eg our local redis metrics. are we planning on moving all of those to prometheus?

given this is a new pattern which no one else has context on, might be good to add a README in CN for our prometheus usage pattern

also the Discprov code changes should be separated into a separate PR, however minor. and someone with Discprov codebase familiarity should review those

SidSethi · 2022-04-11T14:53:14Z

discovery-provider/src/utils/prometheus_metric.py

@@ -58,16 +58,14 @@ def save_time(self, labels=None, start_time=None):
        self.save(self.elapsed(start_time), labels)

    def save(self, value, labels=None):
+        this_metric = self.metric


does this actually work? wouldn't the below assignments just modify the value of this_metric as opposed to updating self.metric?

Yup, it does!

self.metric.labels() returns a new Metric object which we will then use to call .set() or value() with.

We don't seek to change self.metric since we might want to call self.metric.set() later without including any labels, but sometimes we do want to call self.metric.labels(labels).set().

New PR here: #2901

creator-node/test/lib/app.js

creator-node/src/routes/prometheusMetricsExporter.js

creator-node/src/services/PrometheusMetric.js

creator-node/src/routes/prometheusMetricsExporter.js

dmanjunath

+1 to @SidSethi's comments and left a few comments of my own

creator-node/package.json

creator-node/src/routes/prometheusMetricsExporter.js

discovery-provider/src/queries/get_attestation.py

… into jc-asi-983

SidSethi

aside from several implementation related comments, i think it would be good to move the prometheus code into src/services/metrics/* folder and add a README for common usage patterns bc i found it pretty difficult to follow what was happening

creator-node/src/routes/prometheusMetricsExporter.js

SidSethi · 2022-04-28T20:27:10Z

creator-node/src/routes/prometheusMetricsExporter.js

+
+module.exports = function (app) {
+  app.get(
+    '/prometheus_metrics',


can we document what this route is doing
iiuc:

have prometheus populate all metrics into a "collector"

pull info from the collectors via ".metrics()"

would be nice to rewrite this a bit as:

try { // TODO document - what do these do PrometheusMetric.populateCollectors() const data = await Prometheus.register.metrics() // TODO - what/why res.set('Content-Type', Prometheus.register.contentType) return successResponse(data) } catch (e) { console.error('Prometheus Metrics Exporter error:', e) return errorResponseServerError(e.message) }``` take a look at how other routes do response handling e.g. https://github.com/AudiusProject/audius-protocol/blob/jc-asi-983/creator-node/src/routes/tracks.js#L595

is it worth rate limiting or caching response from this route?
not sure how expensive the processing inside this function is

cc @dmanjunath

nah probably not worth doing either tbh. i think prometheus also clears a bunch of metrics automatically once it's collected

@SidSethi could you look at the new layout and confirm that is clearer?

I couldn't use successResponse since that only returns json objects, not raw text.

re: rate limiting, that's something that we discussed looking into if it became a problem by using a "secret api key" specifically for the /prometheus_metrics endpoint.

creator-node/src/services/PrometheusMetric.js

creator-node/src/routes/tracks.js

creator-node/src/services/PrometheusMetric.js

SidSethi · 2022-04-28T20:44:42Z

creator-node/src/services/PrometheusMetric.js

+    // set metric prefix of audius_project_
+    name = `audius_cn_${name}`
+
+    this.metricType = metric_type


can we instead just define the constructor param as

constructor(name, description, labelNames, metric_type = PrometheusType.HISTOGRAM)

actually even better - lets not allow a default and force each caller to explicitly specify the metric type

I moved the constructor to use named parameters, so I believe I've lost the ability to set default parameters, right?

For almost all cases, we'll want histograms since those "time things", but sometimes we'll want gauges to see what the current stat is.

SidSethi · 2022-04-28T21:06:40Z

creator-node/src/services/PrometheusMetric.js

+      this.metricType = PrometheusType.HISTOGRAM
+    }
+
+    // CollectorRegistries must be uniquely named


whats a CollectorRegistry?

We're not allowed to call new PrometheusMetricClass({name}) twice with the same name.

Instead, we will use something like a Python defaultdict to create and return new PrometheusMetricClass({name}) if we haven't seen it before, or the previously created instance if we have seen it before.

A good hierarchical overview is:

Prometheus-Client Registries hold Collectors.

Collectors hold registers of the metrics that they are holding.

Exporters export all metrics held within registries for each collector held within the main registry.

Prometheus-Client provides both Metric Registries as well as an Exporter for the Registries.

Prometheus scrapes Exporters to import all metrics.

creator-node/src/routes/tracks.js

SidSethi · 2022-04-28T21:14:41Z

creator-node/src/services/PrometheusMetric.js

+   */
+  saveTime(labels, startTime) {
+    this.save(this.elapsed(startTime), labels)
+  }


this nested flow is confusing
saveTime calls never specify startTime, so we should define the constructor here as
saveTime(labels, startTime = Date.now())
instead of nesting that default in elapsed()

saveTime is saving the time it took between when the constructor was called (first two lines of the constructor) and when saveTime() is called.

However, you can choose to overload saveTime for different reasons while still relying on the same metric name.

What startTime can't be is Date.now() since we'll want Date.now() - startTime as seen in elapsed().

dmanjunath · 2022-04-28T00:33:46Z

creator-node/src/routes/prometheusMetricsExporter.js

+    handleResponse(async (req, res) => {
+      try {
+        PrometheusMetric.populateCollectors()
+        // res.set('Content-Type', Prometheus.register.contentType)


do we need this line?

dmanjunath · 2022-04-28T00:33:58Z

creator-node/test/prometheus.test.js

+const { getApp } = require('./lib/app')
+const { getLibsMock } = require('./lib/libsMock')
+
+describe('test Users', async function () {


test Prometheus?

dmanjunath · 2022-04-28T23:19:55Z

creator-node/src/routes/prometheusMetricsExporter.js

+
+module.exports = function (app) {
+  app.get(
+    '/prometheus_metrics',


nah probably not worth doing either tbh. i think prometheus also clears a bunch of metrics automatically once it's collected

creator-node/src/routes/prometheusMetricsExporter.js

creator-node/src/routes/tracks.js

SidSethi · 2022-06-14T16:44:02Z

closing in favor of #3166 🙂 thx joaquin for all the help

joaquincasares added 3 commits April 8, 2022 22:02

add PrometheusMetrics to content node

56491bc

manual test fixes

793268d

upgrade black

c0d296f

joaquincasares requested a review from SidSethi April 8, 2022 23:02

joaquincasares assigned dmanjunath and raymondjacobson Apr 8, 2022

pull-request-size bot added the size/L label Apr 8, 2022

joaquincasares changed the title ~~Jc asi 983~~ Add PrometheusClient to ContentNode Apr 8, 2022

joaquincasares added 11 commits April 8, 2022 23:07

lint

54b5bdc

lint

fda5c61

match case

e9a73db

marcus fix #1

6fa21a4

Revert "marcus fix #1"

301ba37

This reverts commit 6fa21a4.

piazza fix #1

5681485

lint

df4ea48

clear before collecting

6adc914

clear with clearRequireCache()

0d2a0d9

no more logs

7231c08

Merge branch 'master' into jc-asi-983

39f1cfc

SidSethi suggested changes Apr 11, 2022

View reviewed changes

SidSethi requested a review from dmanjunath April 11, 2022 15:03

SidSethi unassigned dmanjunath Apr 11, 2022

dmanjunath suggested changes Apr 11, 2022

View reviewed changes

creator-node/package.json Outdated Show resolved Hide resolved

creator-node/src/routes/prometheusMetricsExporter.js Show resolved Hide resolved

discovery-provider/src/queries/get_attestation.py Outdated Show resolved Hide resolved

joaquincasares mentioned this pull request Apr 12, 2022

Simplify logic for common object calls #2901

Merged

joaquincasares added 6 commits April 12, 2022 02:48

remove changes to discprov

2b50034

Merge branch 'jc-asi-983' of github.com:AudiusProject/audius-protocol…

8669fb6

… into jc-asi-983

first pass of requested changes

ddb45cd

first pass of documentation

c745b81

Merge branch 'master' into jc-asi-983

7781314

lint

afec0f5

joaquincasares added 9 commits April 26, 2022 19:08

Merge branch 'master' into jc-asi-983

308f4d4

base test for prometheus

b47af91

add Prometheus ITs

00b0f5a

Merge branch 'master' into jc-asi-983

109ef0f

testing handleResponse

16b59cd

Merge branch 'jc-asi-983' of github.com:AudiusProject/audius-protocol…

8e0ccf2

… into jc-asi-983

Merge branch 'master' into jc-asi-983

89bb91a

use as much of the ../apiHelpers as possible

57b285a

Merge branch 'jc-asi-983' of github.com:AudiusProject/audius-protocol…

8915853

… into jc-asi-983

joaquincasares requested review from SidSethi and dmanjunath April 28, 2022 02:46

joaquincasares added 2 commits April 28, 2022 02:55

use sendResponse

41f6afc

lint

62304d2

SidSethi suggested changes Apr 28, 2022

View reviewed changes

joaquincasares added 6 commits April 28, 2022 22:03

named params

489eb47

metric_type -> metricType

56499ee

reorder constructor

99049dd

lint

9dd7d31

remove handleResponse

95d008e

lint

cecf829

dmanjunath reviewed Apr 29, 2022

View reviewed changes

remove res.set

19acdcd

SidSethi changed the title ~~Add PrometheusClient to ContentNode~~ CON-181 Add PrometheusClient to ContentNode May 31, 2022

SidSethi changed the title ~~CON-181 Add PrometheusClient to ContentNode~~ CON-181 Add PrometheusClient to ContentNode (from joaquin) May 31, 2022

SidSethi changed the title ~~CON-181 Add PrometheusClient to ContentNode (from joaquin)~~ CON-181 v1 Add PrometheusClient to ContentNode (from joaquin) Jun 1, 2022

SidSethi changed the title ~~CON-181 v1 Add PrometheusClient to ContentNode (from joaquin)~~ CON-181 - joaquin - Add PrometheusClient to ContentNode Jun 3, 2022

SidSethi closed this Jun 14, 2022

SidSethi deleted the jc-asi-983 branch June 14, 2022 16:44

AudiusProject deleted a comment from linear bot Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CON-181 - joaquin - Add PrometheusClient to ContentNode #2888

CON-181 - joaquin - Add PrometheusClient to ContentNode #2888

joaquincasares commented Apr 8, 2022

SidSethi left a comment

SidSethi Apr 11, 2022

joaquincasares Apr 12, 2022

dmanjunath left a comment

SidSethi left a comment

SidSethi Apr 28, 2022

SidSethi Apr 28, 2022

dmanjunath Apr 28, 2022

joaquincasares Apr 28, 2022

SidSethi Apr 28, 2022

SidSethi Apr 28, 2022

joaquincasares Apr 28, 2022

SidSethi Apr 28, 2022

joaquincasares Apr 28, 2022 •

edited

Loading

SidSethi Apr 28, 2022

joaquincasares Apr 28, 2022

dmanjunath Apr 28, 2022

dmanjunath Apr 28, 2022

dmanjunath Apr 28, 2022

SidSethi commented Jun 14, 2022

CON-181 - joaquin - Add PrometheusClient to ContentNode #2888

CON-181 - joaquin - Add PrometheusClient to ContentNode #2888

Conversation

joaquincasares commented Apr 8, 2022

Description

Tests

How will this change be monitored? Are there sufficient logs?

SidSethi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanjunath left a comment

Choose a reason for hiding this comment

SidSethi 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

joaquincasares Apr 28, 2022 • edited Loading

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

SidSethi commented Jun 14, 2022

joaquincasares Apr 28, 2022 •

edited

Loading