monitoring functional #26876

snopoke · 2020-03-17T14:07:50Z

Re-take of #26837 that takes a simplified approach.

The class based implementation was much more complex and since most of the calls are single use this approach seemed cleaner.

SUMMARY

Initial implementation of #26816

Initial code structure and classes to allow supporting multiple monitoring backends. Current support for Datadog and Prometheus.

Outstanding work (not for this PR):

Migrate remaining usages of Datadog functions
Enable use of the push gateway for sending metrics from Celery tasks

Corresponding commcare-cloud change: dimagi/commcare-cloud#3721

Co-Authored-By: Daniel Miller <[email protected]>

…/monitoring

This change means we don't have to instantiate a new class when applying the tags which simplifies the class structure a lot.

corehq/apps/receiverwrapper/views.py

corehq/util/metrics/__init__.py

corehq/util/metrics/datadog.py

corehq/util/metrics/metrics.py

corehq/util/metrics/tests/test_metrics.py

deployment/gunicorn/gunicorn_conf.py

esoergel

I like this code structure and the resulting API very much.

corehq/util/metrics/datadog.py

corehq/util/metrics/metrics.py

corehq/util/metrics/__init__.py

millerdev

I like the functional interface, thanks.

corehq/util/metrics/__init__.py

corehq/util/metrics/metrics.py

millerdev · 2020-03-18T02:24:43Z

corehq/util/metrics/views.py

+
+
+def prometheus_metrics(request):
+    """Exports /metrics as a Django view.


What is this view used for? Should it be protected by an admin-only decorator? I'm assuming this is not the only interface for viewing prometheus metrics... seems like it would be bad if metrics became unavailable if this django view became inaccessible for whatever reason.

Because Prometheus is primarily a pull based system the metrics need to be exposed as a view. There are a few options for this:

As a Django view

this can either be protected by a shared secret / basic auth or, since it will be behind nginx in production we can deny access to it via the proxy.

A separate HTTP server run on Django startup

prometheus_client.start_http_server(port, addr=addr)

A completely separate HTTP server

I'm leaning towards 3: f1874e4

Ah, that makes sense.

As a Django view ... since it will be behind nginx in production we can deny access to it via the proxy

I'm -1 on this option if it's not authenticated in some way. It sounds like it may increase the complexity of proxy configuration, and someone who is setting up their own instance of HQ but doesn't know that it should be hidden may inadvertently advertise their metrics to the world.

If we went that route I would update commcare-cloud to have the correct configuration by default. But I think for now I'll remove this view or restrict it's access.

deployment/gunicorn/gunicorn_conf.py

urls.py

snopoke · 2020-03-18T12:15:22Z

I think I'm done with this PR for now and will do further work in a new branch.

esoergel · 2020-03-18T13:57:11Z

corehq/util/metrics/__init__.py

+===========
+
+* All metrics must use the prefix 'commcare.'
+"""


This is great, thanks for adding!

deployment/prometheum_server.py

corehq/util/metrics/__init__.py

corehq/util/metrics/prometheus.py

kaapstorm

I want to echo the feedback on the great interface. I really like the approach you've taken here.

And great docs. In fact, it was the reference to the Datadog tagging docs that made me think that split(':') might need to be a split(':', 1).

corehq/util/metrics/datadog.py

corehq/util/metrics/tests/utils.py

Co-Authored-By: Norman Hooper <[email protected]>

snopoke · 2020-03-19T12:19:49Z

DD metrics changes:

commcare.xform_submissions.count lag tag (histogram)
  -> commcare.xform_submissions.lag.days lag tag 

commcare.xform_submissions.count duration tag (histogram)
  -> commcare.xform_submissions.duration.seconds duration tag

commcare.corrupt-multimedia-submission.error.count (counter)
  -> commcare.corrupt_multimedia_submissions

millerdev · 2020-03-19T15:58:52Z

corehq/util/metrics/prometheus.py

+    def _histogram(self, name: str, value: float, bucket_tag: str, buckets: List[int], bucket_unit: str = '',
+                  tags: dict = None, documentation: str = ''):
+        """
+        A cumulative histogram with a base metric name of <basename> exposes multiple time series


You missed the one on this line

Suggested change

A cumulative histogram with a base metric name of <basename> exposes multiple time series

A cumulative histogram with a base metric name of <name> exposes multiple time series

Thanks: #26916

snopoke and others added 30 commits March 9, 2020 14:39

move datadog init to AppConfig.ready

90bdf7d

pluggable metrics

7ad93fa

add histogram and tests

0694926

bucket values less than or equal

c1f8270

add prometheus client to requirements

5034156

Merge branch 'master' into sk/monitoring

172ef37

make metrics lazy

2be4e6a

example histogram

eb8afe3

Merge branch 'master' into sk/monitoring

54e91c0

convert sumbission metrics

54f2d1b

docstrings

eba69c4

stickler

01fbc07

keep tag_values as dict instead of splitting and re-combining

450f211

update links

2c3edef

remove unnecessary list

8664161

Co-Authored-By: Daniel Miller <[email protected]>

replace typle() with ()

0bcedea

Merge branch 'sk/monitoring' of github.com:dimagi/commcare-hq into sk…

9ba3d4a

…/monitoring

fix tags

904e358

pass other args

d68e104

revert change to datadog bucketing boundry

5c00052

remove unnecessary list

e00b7e1

apply tags at the same time as recording the metric

20e8a61

This change means we don't have to instantiate a new class when applying the tags which simplifies the class structure a lot.

dummy metric

0ab0006

functional interface

02b40ee

re-do configuration via settings

31cfba9

move initialization into provider

892e57b

replace datadog_gauge

dbdbd3f

instantiate provider

5897eb9

hook up metrics view

28a8a5b

todo

abd0355

stickler-ci reviewed Mar 17, 2020

View reviewed changes

snopoke mentioned this pull request Mar 17, 2020

metrics providers dimagi/commcare-cloud#3721

Merged

snopoke added the Open for review: do not merge A work in progress label Mar 17, 2020

lint

75fa979

snopoke mentioned this pull request Mar 17, 2020

[wip] prometheus monitoring support #26837

Closed

esoergel approved these changes Mar 17, 2020

View reviewed changes

millerdev reviewed Mar 18, 2020

View reviewed changes

snopoke added 8 commits March 18, 2020 10:11

PR feedback

017ca5a

log output from DebugMetrics

770f4a0

move metrics view to hq/admin

8bb9e03

move metrics_gauge_task to package init

9a49d26

fix import

74ce07f

add script for running metrics endpoint

f1874e4

update docs

d4220f0

docs

0457c00

esoergel approved these changes Mar 18, 2020

View reviewed changes

do setup in __init__

3d79912

millerdev approved these changes Mar 18, 2020

View reviewed changes

deployment/prometheum_server.py Outdated Show resolved Hide resolved

corehq/util/metrics/__init__.py Outdated Show resolved Hide resolved

corehq/util/metrics/prometheus.py Show resolved Hide resolved

snopoke added 2 commits March 19, 2020 10:52

simplify prometheus server

d4c3dc1

doc updates

1017c85

kaapstorm approved these changes Mar 19, 2020

View reviewed changes

corehq/util/metrics/datadog.py Outdated Show resolved Hide resolved

corehq/util/metrics/tests/utils.py Outdated Show resolved Hide resolved

Apply suggestions from code review

d18075d

Co-Authored-By: Norman Hooper <[email protected]>

snopoke removed the Open for review: do not merge A work in progress label Mar 19, 2020

snopoke merged commit b5b3fba into master Mar 19, 2020

snopoke deleted the sk/monitoring-functional branch March 19, 2020 11:58

snopoke mentioned this pull request Mar 19, 2020

only enable metrics view in DEBUG mode #26903

Merged

millerdev reviewed Mar 19, 2020

View reviewed changes

dannyroberts mentioned this pull request Mar 19, 2020

Revert breaking prometheus changes to gunicorn_conf.py #26911

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monitoring functional #26876

monitoring functional #26876

snopoke commented Mar 17, 2020 •

edited

Loading

esoergel left a comment

millerdev left a comment

millerdev Mar 18, 2020

snopoke Mar 18, 2020

millerdev Mar 18, 2020

snopoke Mar 19, 2020

snopoke Mar 19, 2020

snopoke commented Mar 18, 2020

esoergel Mar 18, 2020

kaapstorm left a comment

snopoke commented Mar 19, 2020 •

edited

Loading

millerdev Mar 19, 2020

snopoke Mar 20, 2020



		def prometheus_metrics(request):
		"""Exports /metrics as a Django view.

	A cumulative histogram with a base metric name of <basename> exposes multiple time series
	A cumulative histogram with a base metric name of <name> exposes multiple time series

monitoring functional #26876

monitoring functional #26876

Conversation

snopoke commented Mar 17, 2020 • edited Loading

SUMMARY

esoergel left a comment

Choose a reason for hiding this comment

millerdev 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

snopoke commented Mar 18, 2020

Choose a reason for hiding this comment

kaapstorm left a comment

Choose a reason for hiding this comment

snopoke commented Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snopoke commented Mar 17, 2020 •

edited

Loading

snopoke commented Mar 19, 2020 •

edited

Loading