[wip] prometheus monitoring support #26837

snopoke · 2020-03-11T15:17:04Z

This isn't done yet but putting up for initial review.

SUMMARY

Implementation of #26816

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

I've converted a few metrics to see how it would work. Further notes in the code.

Missing pieces:

the views to expose the metrics need to be added
Prometheus initialisation
Gunicorn hooks: https://github.com/prometheus/client_python#multiprocess-mode-gunicorn

Migration plan:

this can be done piecemeal so once the basics are done I'll start with that
there will likely be some decisions to make about changes to the metrics
- split histograms out into their own metrics
- should we adhere to a naming convention (e.g. https://prometheus.io/docs/practices/naming/)

corehq/util/datadog/apps.py

corehq/util/metrics/__init__.py

corehq/util/metrics/metrics.py

snopoke · 2020-03-11T15:18:49Z

corehq/apps/receiverwrapper/views.py


-    datadog_counter('commcare.xform_submissions.count', tags=tags)
+    submission_counter.tag(**tags).inc()


This is an example of a metric where we have attached multiple histogram bucket tags to a single metric. If we want to convert these histograms to the native histogram types then they must be separated out as is done here.

corehq/util/datadog/utils.py

snopoke · 2020-03-11T15:23:03Z

corehq/util/metrics/__init__.py

+    return enabled
+
+
+metrics = DelegatedMetrics(SimpleLazyObject(_get_metrics))  # singleton/global


Initially I had this as a function get_metrics() but after making it lazy there wasn't a need for the function. This will only call the _get_metrics function when a metric class is actually used. This is needed make sure that Django is fully set up before determining if the metrics providers are enabled (since they rely on settings).

m = metrics.counter('name', 'description') # still lazy m.inc() # now it's evaluated

Does this mean that DelegatedMetrics is always used even when there is only a single "metrics" object to delegate to? That seems wasteful. Would it be possible to only delegate if there is more than one (I would expect that to be the exceptional case).

Yes it does mean that. The alternative is to split DelegatedMetrics into two classes: DelegatedMetrics and LazyMetrics and then combine them as needed (the laziness is always needed).

snopoke · 2020-03-11T15:23:55Z

corehq/util/metrics/datadog.py

+        _datadog_record(statsd.gauge, self.name, value, tags)
+
+
+class Histogram(HqHistogram):


As stated in the docstring this isn't a real histogram. It uses our workaround of adding bucketed tags.

snopoke · 2020-03-11T15:24:24Z

corehq/util/metrics/metrics.py

+        "Did you mean to call your metric 'commcare.{}'? ".format(name))
+
+
+def _validate_tag_names(tag_names):


This is required by Prometheus.

snopoke · 2020-03-11T15:24:58Z

corehq/util/metrics/metrics.py

+        self._record(value)
+
+
+DEFAULT_BUCKETS = (.005, .01, .025, .05, .075, .1, .25, .5, .75, 1.0, 2.5, 5.0, 7.5, 10.0, INF)


The default in Prometheus

snopoke · 2020-03-11T15:41:31Z

corehq/apps/receiverwrapper/views.py

 from corehq.util.timer import TimingContext

 PROFILE_PROBABILITY = float(os.getenv('COMMCARE_PROFILE_SUBMISSION_PROBABILITY', 0))
 PROFILE_LIMIT = os.getenv('COMMCARE_PROFILE_SUBMISSION_LIMIT')
 PROFILE_LIMIT = int(PROFILE_LIMIT) if PROFILE_LIMIT is not None else 1

+corrupt_multimedia_counter = metrics.counter(
+    'commcare.corrupt_multimedia_submissions', 'Count of requests with corrupt multimedia',


This name is changed

- commcare.corrupt-multimedia-submission.error.count + commcare.corrupt_multimedia_submissions

I changed this because .error seemed redundant since it's implied by the fact that it's 'corrupt'. I dropped the 'count' suffix because Prometheus adds a _total prefix to the end of counter metric names. If we want to keep the suffix we should add it to all 'counter' metrics at the provider level.

millerdev

How was Prometheus chosen? Is it the clear leader in the open source monitoring space, or are there other options?

About the naming convention, it looks like all of our current metric names violate the Prometheus default since they use dot (.) as a separator (they also use underscore (_) as sub-separator), where Prometheus uses underscore only. Do I have that right?

millerdev · 2020-03-12T01:12:41Z

corehq/util/metrics/datadog.py

+    _gauge_class = Gauge
+
+    def enabled(self) -> bool:
+        return api._api_key and api._application_key


this doesn't look like it will always return bool (it will probably return a string or None). If you want a bool you could do

return bool(api._api_key and api._application_key)

But that doesn't seem very Pythonic. I'd probably drop the type hint.

However, I'm not very familiar with the conventions around type hints. Does -> bool imply that returned object can be treated like a bool (most objects support that), or that the returned value is always True or False? This is one reason I'm not a fan of type hints in Python.

millerdev · 2020-03-12T01:17:41Z

corehq/util/metrics/metrics.py

+
+METRIC_NAME_RE = re.compile(r'^[a-zA-Z_:.][a-zA-Z0-9_:.]*$')
+METRIC_TAG_NAME_RE = re.compile(r'^[a-zA-Z_][a-zA-Z0-9_]*$')
+RESERVED_METRIC_TAG_NAME_RE = re.compile(r'^__.*$')


Can this be simplified?

Suggested change

RESERVED_METRIC_TAG_NAME_RE = re.compile(r'^__.*$')

RESERVED_METRIC_TAG_NAME_RE = re.compile(r'^__')

True, this was copied from the Prometheus client.

corehq/util/metrics/metrics.py

millerdev · 2020-03-12T01:40:11Z

corehq/util/metrics/prometheus.py

+        return self.__class__(self.name, self.documentation, self.tag_names, self.tag_values, delegate)
+
+
+class Counter(PrometheusMetricBase, HqCounter):


Oof, multiple inheritance with diamonds... I don't love this.

I'd rather have simple functional call patterns like our existing datadog_counter(..., tags=...) (made generic so as not to have "datadog" in the name) rather than changing all of our metrics calls to use a new metrics system with extensive use of inheritance.

Yes, the inheritance isn't great here. The alternative is a bit more duplicate code but not a lot. I'll see what I can do to make it better if we stay on this course.

If we want to keep it functional then we would need to have a centralized metrics singleton that would always return the same metric class when called (in the case of Pometheus). It would also mean that every call to the metric would need to pass in all required args (which is the case now anyway):

metrics_histogram(value, tags=..., buckets=..., unit=..., dd_bucket_tag=...)

I've resolved this by removing the need to create new copies of the class when applying tags:

- metric.tag(group='a').inc() + metric.inc(group='a')

corehq/util/metrics/datadog.py

corehq/util/datadog/utils.py

millerdev · 2020-03-12T02:27:52Z

corehq/util/metrics/__init__.py

+    return enabled
+
+
+metrics = DelegatedMetrics(SimpleLazyObject(_get_metrics))  # singleton/global


Does this mean that DelegatedMetrics is always used even when there is only a single "metrics" object to delegate to? That seems wasteful. Would it be possible to only delegate if there is more than one (I would expect that to be the exceptional case).

corehq/util/metrics/metrics.py

corehq/util/metrics/tests/utils.py

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

…/monitoring

snopoke · 2020-03-12T12:11:44Z

How was Prometheus chosen? Is it the clear leader in the open source monitoring space, or are there other options?

We did a basic survey. There a few options but Prometheus appears to be the more common one and support all the use cases we need.

About the naming convention, it looks like all of our current metric names violate the Prometheus default since they use dot (.) as a separator (they also use underscore (_) as sub-separator), where Prometheus uses underscore only. Do I have that right?

That's correct. I've already taken that into account by replacing . with _ for Prometheus so I'm not suggesting we change that. I was meaning more of the semantic guidelines.

stickler-ci · 2020-03-12T14:54:21Z

corehq/util/metrics/tests/test_metrics.py

+from django.utils.functional import SimpleLazyObject
+
+from corehq.util.metrics import DatadogMetrics, PrometheusMetrics
+from corehq.util.metrics.metrics import (


F401 'corehq.util.metrics.metrics.HqHistogram' imported but unused

snopoke · 2020-03-12T15:51:10Z

Note to reviewers: I'm offline till Monday. I'd like to schedule a call for when I'm back to decide on a course of action for this. If you'd like to weigh in please review by then (9 am EST on 16 March).

Points of discussion:

Class based vs functional API

counter = metrics.counter(name, tag_names=['a'])
counter.tag(a='tag').inc(2)

vs

metrics_counter(name, 2, tags={'a': 'tag'})

whether we should try and standardise the metric naming and usage at all
- https://prometheus.io/docs/practices/naming/#metric-names (note we can still have 'a.b' if we want for DD metrics

corehq/util/metrics/tests/utils.py

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

stickler-ci · 2020-03-16T11:48:41Z

corehq/util/metrics/metrics.py

+class HqHistogram(MetricBase):
+
+    def __init__(self, name: str, documentation: str,
+                  bucket_tag: str, buckets: List[int] = DEFAULT_BUCKETS, bucket_unit: str = '',


E127 continuation line over-indented for visual indent

stickler-ci · 2020-03-16T11:48:41Z

corehq/util/metrics/metrics.py

+
+    def __init__(self, name: str, documentation: str,
+                  bucket_tag: str, buckets: List[int] = DEFAULT_BUCKETS, bucket_unit: str = '',
+                  tag_names: Iterable = ()):


E127 continuation line over-indented for visual indent

stickler-ci · 2020-03-16T11:48:41Z

corehq/util/metrics/prometheus.py

@@ -0,0 +1,59 @@
+import settings
+from corehq.util.metrics.metrics import (


F401 'corehq.util.metrics.metrics.MetricBase' imported but unused

snopoke · 2020-03-17T14:39:09Z

closing in favour of #26876

snopoke added 10 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

snopoke requested review from esoergel and proteusvacuum as code owners March 11, 2020 15:17

stickler-ci reviewed Mar 11, 2020

View reviewed changes

docstrings

eba69c4

snopoke commented Mar 11, 2020

View reviewed changes

stickler

01fbc07

snopoke requested review from millerdev, czue, calellowitz and dannyroberts March 11, 2020 15:34

snopoke added the product/invisible Change has no end-user visible impact label Mar 11, 2020

snopoke commented Mar 11, 2020

View reviewed changes

millerdev reviewed Mar 12, 2020

View reviewed changes

snopoke and others added 5 commits March 12, 2020 11:17

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

snopoke added 2 commits March 12, 2020 15:34

pass other args

d68e104

revert change to datadog bucketing boundry

5c00052

stickler-ci reviewed Mar 12, 2020

View reviewed changes

millerdev reviewed Mar 12, 2020

View reviewed changes

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

snopoke added 2 commits March 16, 2020 13:40

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.

stickler-ci reviewed Mar 16, 2020

View reviewed changes

dummy metric

0ab0006

snopoke mentioned this pull request Mar 17, 2020

monitoring functional #26876

Merged

snopoke closed this Mar 17, 2020

snopoke deleted the sk/monitoring branch March 17, 2020 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] prometheus monitoring support #26837

[wip] prometheus monitoring support #26837

snopoke commented Mar 11, 2020 •

edited

Loading

snopoke Mar 11, 2020

snopoke Mar 11, 2020

millerdev Mar 12, 2020

snopoke Mar 12, 2020

snopoke Mar 11, 2020

snopoke Mar 11, 2020

snopoke Mar 11, 2020

snopoke Mar 11, 2020

millerdev left a comment

millerdev Mar 12, 2020

millerdev Mar 12, 2020

snopoke Mar 12, 2020

millerdev Mar 12, 2020

snopoke Mar 12, 2020

snopoke Mar 16, 2020

millerdev Mar 12, 2020

snopoke commented Mar 12, 2020

stickler-ci Mar 12, 2020

snopoke commented Mar 12, 2020 •

edited

Loading

stickler-ci Mar 16, 2020

stickler-ci Mar 16, 2020

stickler-ci Mar 16, 2020

snopoke commented Mar 17, 2020


		datadog_counter('commcare.xform_submissions.count', tags=tags)
		submission_counter.tag(**tags).inc()

		return enabled


		metrics = DelegatedMetrics(SimpleLazyObject(_get_metrics)) # singleton/global

		_datadog_record(statsd.gauge, self.name, value, tags)


		class Histogram(HqHistogram):

		"Did you mean to call your metric 'commcare.{}'? ".format(name))


		def _validate_tag_names(tag_names):

		self._record(value)


		DEFAULT_BUCKETS = (.005, .01, .025, .05, .075, .1, .25, .5, .75, 1.0, 2.5, 5.0, 7.5, 10.0, INF)

	RESERVED_METRIC_TAG_NAME_RE = re.compile(r'^__.*$')
	RESERVED_METRIC_TAG_NAME_RE = re.compile(r'^__')

		return self.__class__(self.name, self.documentation, self.tag_names, self.tag_values, delegate)


		class Counter(PrometheusMetricBase, HqCounter):

		@@ -0,0 +1,59 @@
		import settings
		from corehq.util.metrics.metrics import (

[wip] prometheus monitoring support #26837

[wip] prometheus monitoring support #26837

Conversation

snopoke commented Mar 11, 2020 • edited Loading

SUMMARY

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snopoke commented Mar 12, 2020

Choose a reason for hiding this comment

snopoke commented Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snopoke commented Mar 17, 2020

snopoke commented Mar 11, 2020 •

edited

Loading

snopoke commented Mar 12, 2020 •

edited

Loading