⚙ D1321 swh-storage: Install counter metrics

Harbormaster completed remote builds in B4957: Diff 4194.Mar 29 2019, 6:09 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/243/ for more details.

ardumont edited the summary of this revision. (Show Details)Mar 29 2019, 6:13 PM

ardumont added inline comments.Mar 29 2019, 6:15 PM

swh/storage/api/server.py
24	The idea here is to have a mapping per endpoint with the interesting key we want to use as metric. That might be also used in the storage.py module. The commented list is because we could need more information that just one bare key. As i'm already unsure of the initial implementation, i'm going for a simple one first.
36	Implementation detail to have access of the data prior to any `Response` wrapping the encode_data does. Again with the discussion about the grpc [1] which could ease our life. [1] https://grpc.io/docs/guides/#overview

ardumont planned changes to this revision.Mar 29 2019, 6:18 PM

ardumont retitled this revision from POC: Install counter metrics to Install counter metrics: Implementation up for discussion.Mar 29 2019, 6:24 PM

api.server: Update docstrings on content_add to explicit the output
api.server: Remove encode step from metrics
storage: Update tests to check the summary result act as expected
storage.in_memory.content_add: Adapt implementation to send summary
storage*.directory_add: Return summary
api.server.directory_add: Add counter metrics
content_add: Normalize the summary keys
directory_add: Normalize metrics keys
storage: Align implementations to compute correct metrics
storage*.revision_add: Return summary
storage.in_memory: Fix missing filtering on release_add
storage*.release_add: Return summary
storage*.snapshot_add: Return summary

Remains to actually exploit the summary appropriately in the
swh.storage.api.server module (in-progress)

ardumont planned changes to this revision.Apr 1 2019, 4:28 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/269/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/269/console

Harbormaster failed remote builds in B4996: Diff 4231!Apr 1 2019, 4:29 PM

Rebase on latest master

Fix flake8 warnings that broke the ci...

Harbormaster failed remote builds in B5001: Diff 4235!Apr 1 2019, 6:03 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/271/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/271/console

ardumont added inline comments.Apr 1 2019, 6:05 PM

sql/upgrades/131.sql
10 ↗	(On Diff #4235)	What?

ardumont planned changes to this revision.Apr 1 2019, 6:05 PM

Update my local master who were late. So the previous diff version
contains too much data

ardumont planned changes to this revision.Apr 1 2019, 6:08 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/272/ for more details.

Harbormaster completed remote builds in B5002: Diff 4236.Apr 1 2019, 6:12 PM

ardumont retitled this revision from Install counter metrics: Implementation up for discussion to Install counter metrics.Apr 1 2019, 11:17 PM

ardumont edited the summary of this revision. (Show Details)

ardumont added inline comments.Apr 2 2019, 5:22 PM

swh/storage/api/server.py
53	Right now, that gives something like: U 127.0.0.1:47068 -> 127.0.0.1:8125 swh_storage_request_object_count:{'snapshot_added': 1}\|c\|#endpoint:snapshot_add

swh.storage: Adapt and unify _add endpoints
api.server: Exploit new endpoint to compute metrics

Adapt according to our latest exchange

Metrics now looks like:

U 127.0.0.1:55331 -> 127.0.0.1:8125
  swh_storage_content_add:3681|c|#endpoint:content_add,object_type:content,operation:add
#
U 127.0.0.1:57664 -> 127.0.0.1:8125
  swh_storage_skipped_content_add:0|c|#endpoint:content_add,object_type:skipped_content,operation:add
#
U 127.0.0.1:33871 -> 127.0.0.1:8125
  swh_storage_revision_add:2267|c|#endpoint:revision_add,object_type:revision,operation:add

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/273/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/273/console

Harbormaster failed remote builds in B5022: Diff 4254!Apr 2 2019, 6:51 PM

Fix typo

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/274/ for more details.

Harbormaster completed remote builds in B5023: Diff 4255.Apr 2 2019, 6:59 PM

ardumont retitled this revision from Install counter metrics to swh-storage: Install counter metrics.Apr 2 2019, 11:24 PM

ardumont added a project: Storage manager.

Looks good, thanks! Just a few more changes and this should be all good to merge.

It might make sense to rebase this on top of D1329 when it lands

swh/storage/api/server.py
42	hehe, thanks
45	maybe this would be better if called `process_metrics` or `send_metrics_from_retval`?
52	We should skip all of this processing if value is 0
57	The metric name should be constant with respect to what we're putting in the labels : `swh_storage_operations_total`.
61	In this case we do want the unit in the metric name: `"swh_storage_operations_%s_total" % unit`. (reference: https://prometheus.io/docs/practices/naming/)
91–92	we should really `@time` the `@encode`ing process by swapping the decorators.
swh/storage/storage.py
194–196 ↗	(On Diff #4255)	There's all kinds of fun considerations when considering e.g. the multiplexing objstorage here...

This revision now requires changes to proceed.Apr 2 2019, 11:42 PM

ardumont marked 5 inline comments as done.Apr 3 2019, 12:05 PM

ardumont added inline comments.

swh/storage/api/server.py
91–92	Ok, i thought we did not want to. But thinking about it more, that makes sense to indeed.
swh/storage/storage.py
194–196 ↗	(On Diff #4255)	Well, sure but then, separation of concern, that's the objstorage's job, ain't it? It's fine to keep that as a readme for now ;)

Adapt according to olasd's review:

api.server: Include the encoding step when timing
api.server: Rename to process_metrics function name
api.server.process_metrics: Skip processing when value is 0
server.api: Fix metric names respecting prometheus conventions [1]

Improvments:

api.server: Add metrics to origin_visit_add
api.server: Add metrics on origin_add and origin_add_one
server: Add metrics on origin_metadata_add & metadata_provider_add
storage: Make tool_add endpoint return a list instead of a generator
server: Add metrics on tool_add endpoint

Unrelated (too lazy to open another diff, tell me if you really want me too):

storage: Fix typo in variable name
api.{client,server}: Remove dead code about entity

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/279/ for more details.

Harbormaster completed remote builds in B5035: Diff 4268.Apr 3 2019, 12:13 PM

Heads up, will need to be rebased on D1329 when it lands.
This will need adaptation for the snapshot_add endpoints will take a list of snapshots as input (instead of 1).

ardumont added a child revision: D1337: core.loader: Adapt according to latest storage api change.Apr 3 2019, 2:20 PM

Rebase on latest master

ardumont mentioned this in D1337: core.loader: Adapt according to latest storage api change.Apr 3 2019, 3:10 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/282/ for more details.

Harbormaster completed remote builds in B5046: Diff 4279.Apr 3 2019, 3:13 PM

If we're making metric names constants, this should happen for all of them so that they are defined in the same place.

Maybe something like

OPERATIONS_METRIC = "swh_storage_operations_total"
OPERATIONS_UNIT_METRIC = "swh_storage_operations_{unit}_total"
DURATION_METRIC = "swh_storage_request_duration_seconds"

Then fill the pattern when using the metric which uses the unit.

I'm not entirely convinced by the introduction of the send_metrics indirection as is, using the statsd API itself would be much clearer.

To make it more explicit:

its name should be singular (it sends a single metric)
all its callers should use keyword arguments
it would probably make sense to move the dict key parsing/metric name stuff inside that function, which will help if we ever end up adding an "extrinsic" metric that would use a unit instead of a count.

So the calls would end up looking like:

send_metric(metric='origin:add', count=len(origins), method_name='origin_add')

instead of

send_metrics(len(origins), 'origin_add', 'origin', 'add')

This revision now requires changes to proceed.Apr 3 2019, 3:30 PM

If we're making metric names constants, this should happen for all of them so that they are defined in the same place.
... fill the pattern when using the metric which uses the unit.

right, done.