Metrics documentation update #171

michalwski · 2020-06-16T11:40:36Z

This PR is primarily about metrics documentation update. It also contains the following changes which may be extracted to separate PR based on review comments.

Start the MongoosePush.Metrics.TelemetryMetrics child befor others to captuer events at startup
Short metrics documentation in the code
Add example Prometheus config for MongoosePush and docker-compose file to help with a basic setup.

Assuming that MongoosePush was started with the following command: MIX_ENV=integration mix do test.env.up You can start Prometheus configured to monitor MongoosePush in container executing the command below: docker-compose -f test/docker/docker-compose.prometheus.yml up -d

This is to capture some of the events emitted when APNS or FCM services start

NelsonVides

Fantastic!

The only thing I'd say is that MPush docs, i.e., a single README, is getting out of control with the size, but reorganising that is on a different task 🙂

Oh, and some articles were missing 😛

lib/mongoose_push/metrics/telemetry_metrics.ex

README.md

NelsonVides · 2020-06-16T12:59:58Z

lib/mongoose_push/application.ex

    children =
-      service_children() ++ [MongoosePushWeb.Endpoint, MongoosePush.Metrics.TelemetryMetrics]
+      [MongoosePush.Metrics.TelemetryMetrics] ++ service_children() ++ [MongoosePushWeb.Endpoint]


Good catch 🙂

lib/mongoose_push/metrics/telemetry_metrics.ex

kmakiela

Great update! However, reading through 600 lines of readme is a bit much so that may be something we want to change in the near future.

kmakiela · 2020-06-16T13:24:37Z

README.md

+
+* `mongoose_push_supervisor_init_count{service=${SERVICE}}` - Counts number of push notification service supervisor starts.
+  The `SERVICE` variable can take `"apns"` or `"fcm"` as a value.
+  This metrics is update when MongoosePush starts or later when the underlying supervision tree is terminated and the error is propagate to the main application supervisor.


Suggested change

This metrics is update when MongoosePush starts or later when the underlying supervision tree is terminated and the error is propagate to the main application supervisor.

This metric is updated when MongoosePush starts or later when the underlying supervision tree is terminated and the error is propagated to the main application supervisor.

kmakiela · 2020-06-16T13:29:51Z

test/docker/docker-compose.mpush.yml

@@ -1,5 +1,5 @@
 # This file needs to be used along with `docker-compose.mocks.yml`:
-# docker-compose -f test/docker/docker-compose.mocks.yml -f test/docker/docker-compose.mpush.yml ...
+# PRIV=priv docker-compose -f test/docker/docker-compose.mocks.yml -f test/docker/docker-compose.mpush.yml ...


Why do we need this change?

The docker-compose-mpush.yml requires the PRIV env var. It's set for us when we do mix text.env.up but when running from console we need to export it manually.

michalwski · 2020-06-16T15:12:33Z

@NelsonVides @kmakiela thanks for the review. I 100% agree with the documentation split/refactoring. This is definitely a subject for a separate PR.

janciesla8818

Good stuff! I really like the PR, we just need to make sure that the part regarding less or equal is accurate as it can be a bit confusing.

janciesla8818 · 2020-06-16T16:12:15Z

README.md

+* `LE` defines the `less or equal` values for buckets, currently `1000`, `10_000`, `25_000`, `50_000`, `100_000`, `250_000`, `500_000`, `1000_000` or `+Inf`
+
+> **NOTE**
+> A mesurment of value 50_000 will be added to all buckets which are less or equal to 50_000.


Not sure if I understand this right, I think this is the other way around. The bucket contains measurements that are less or equal to the bucket size.

More particularly they're counters which form a cumulative histogram, le stands for less than or equal to. So 26688 requests took less than or equal to 200ms, 27760 requests took less than or equal to 400ms, and there were 28860 requests in total.
https://www.robustperception.io/how-does-a-prometheus-histogram-work

Right, I messed with that. Good catch!

janciesla8818 · 2020-06-16T16:23:23Z

README.md

+```
+
+The above command assumes that MongoosePush runs on `localhost` and listens on port `8443`.
+Please, mind the `HTTPS` protocol, metrics are hosted on the same port as other API.


Suggested change

Please, mind the `HTTPS` protocol, metrics are hosted on the same port as other API.

Please, mind the `HTTPS` protocol, metrics are hosted on the same port as other API endpoints.

How about this as compromise?
the same as seems more grammatically correct than the same than

lib/mongoose_push/metrics/telemetry_metrics.ex

janciesla8818 · 2020-06-16T16:32:34Z

lib/mongoose_push/metrics/telemetry_metrics.ex

+      Telemetry.Metrics.counter(
+        "mongoose_push.apns.state.get_default_topic.count",


Suggested change

Telemetry.Metrics.counter(

"mongoose_push.apns.state.get_default_topic.count",

Telemetry.Metrics.counter("mongoose_push.apns.state.get_default_topic.count",

just for consistency with the remaining events.

michalwski · 2020-06-16T18:51:59Z

@janciesla8818 thanks for the review. I think I applied all the comments now.

Co-authored-by: Nelson Vides <nelson.vides@erlang-solutions.com>

michalwski added 7 commits June 16, 2020 10:19

fix link to docker compose file in README

9a8af98

Update metrics doc - tips for configuring Prometheus

e775072

Document the notification sent time histogram metric

1f3ce76

Start the MongoosePush.Metrics.TelemetryMetrics before others

c57d94a

This is to capture some of the events emitted when APNS or FCM services start

Update doc with counter metrics

777b018

Add short metrics description in the code

03f6606

michalwski requested review from janciesla8818, leszke and kmakiela June 16, 2020 11:41

NelsonVides reviewed Jun 16, 2020

View reviewed changes

kmakiela reviewed Jun 16, 2020

View reviewed changes

michalwski force-pushed the mp/metrics-doc-update branch from bfd3005 to 1d4a1aa Compare June 16, 2020 15:19

janciesla8818 requested changes Jun 16, 2020

View reviewed changes

michalwski force-pushed the mp/metrics-doc-update branch from 1d4a1aa to 65326aa Compare June 16, 2020 18:50

Apply suggestions from code review

7e7e451

Co-authored-by: Nelson Vides <nelson.vides@erlang-solutions.com>

janciesla8818 force-pushed the mp/metrics-doc-update branch from 65326aa to 7e7e451 Compare June 17, 2020 09:31

janciesla8818 approved these changes Jun 17, 2020

View reviewed changes

NelsonVides approved these changes Jun 17, 2020

View reviewed changes

NelsonVides merged commit e4caac2 into master Jun 17, 2020

NelsonVides deleted the mp/metrics-doc-update branch June 17, 2020 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics documentation update #171

Metrics documentation update #171

michalwski commented Jun 16, 2020

NelsonVides left a comment

NelsonVides Jun 16, 2020

kmakiela left a comment

kmakiela Jun 16, 2020

kmakiela Jun 16, 2020

michalwski Jun 16, 2020

michalwski commented Jun 16, 2020

janciesla8818 left a comment

janciesla8818 Jun 16, 2020

michalwski Jun 16, 2020

janciesla8818 Jun 16, 2020

janciesla8818 Jun 16, 2020

michalwski commented Jun 16, 2020

	This metrics is update when MongoosePush starts or later when the underlying supervision tree is terminated and the error is propagate to the main application supervisor.
	This metric is updated when MongoosePush starts or later when the underlying supervision tree is terminated and the error is propagated to the main application supervisor.

	Please, mind the `HTTPS` protocol, metrics are hosted on the same port as other API.
	Please, mind the `HTTPS` protocol, metrics are hosted on the same port as other API endpoints.

		Telemetry.Metrics.counter(
		"mongoose_push.apns.state.get_default_topic.count",

	Telemetry.Metrics.counter(
	"mongoose_push.apns.state.get_default_topic.count",
	Telemetry.Metrics.counter("mongoose_push.apns.state.get_default_topic.count",

Metrics documentation update #171

Metrics documentation update #171

Conversation

michalwski commented Jun 16, 2020

NelsonVides left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmakiela 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

michalwski commented Jun 16, 2020

janciesla8818 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

michalwski commented Jun 16, 2020