task(Observability): Introduce prometheus-flask-exporter and enable monitoring for presigned urls #864

themarcelor · 2021-01-21T00:59:35Z

Jira Ticket: PXP-7422

Exporting metrics from Fence so we can observe events in our Grafana dashboards.

Result:

qa-dcp@dev_admin:~$ kc exec -it presigned-url-fence-deployment-664d476489-vt6m4 -- curl http://127.0.0.1/metrics | tail -n3
# HELP pre_signed_url_req_total Multiprocess metric
# TYPE pre_signed_url_req_total counter
pre_signed_url_req_total{file_id="b9c368cd-d681-4d42-b9d1-1778c41fc1f7",username="marcelo@email.com"} 4.0

Adding new dependencies:

prometheus_client = "^0.5.0"
prometheus-flask-exporter = "^0.18.1"

Enabling uwsgi stats http server.
Adding a prometheus Counter to the Presigned URL function.
Tracking the username and the file_id for each presigned url request.

Additional info:

The Disk space usage is negligible. The metrics & labels remain the same, the only thing that changes is the value of each metric (counter / gauges / histograms):

$ kc exec -it presigned-url-fence-deployment-594bc9f4cb-pf4d7 -- bash
...
bash-4.4# du -sh /var/www/metrics/metrics.txt
16.0K	/var/www/metrics/metrics.txt
bash-4.4# du -sh /var/tmp/uwsgi_flask_metrics/
56.0K	/var/tmp/uwsgi_flask_metrics/

Running unit tests:

fence [chore/introduce_prometheus_counter_for_presigned_urls●] % poetry run pytest -vv tests/data/test_data.py::test_indexd_download_file

This change is linked with uc-cdis/cloud-automation#1348.
This change is blocked by: uc-cdis/cloud-automation#1498

New Features

Exporting metrics from Fence so we can observe events in our Grafana dashboards.

…onitoring for PreSigned URLs

github-actions · 2021-01-21T01:00:42Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2021-01-21T01:03:52Z

Pull Request Test Coverage Report for Build 10942

18 of 18 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.06%) to 70.498%

Totals
Change from base Build 10885:	0.06%
Covered Lines:	5955
Relevant Lines:	8447

💛 - Coveralls

…ined

paulineribeyre · 2021-01-22T21:28:42Z

clear_prometheus_multiproc

@@ -0,0 +1,7 @@
+#!/bin/bash


comments to explain what this script does and why we need it would be useful

Good call. Let me push a commit.

Could the work in this script be done in the Dockerfile?

This decision was based on research around best practices for prometheus + uwsgi monitoring:
https://www.metricfire.com/blog/monitoring-python-web-app/

.travis.yml

deployment/uwsgi/uwsgi.ini

dockerrun.bash

pyproject.toml

Dockerfile

fence/blueprints/data/indexd.py

tests/data/test_data.py

…signed_urls

Co-authored-by: Pauline Ribeyre <ribeyre@uchicago.edu>

fence/blueprints/data/indexd.py

…of github.com:uc-cdis/fence into chore/introduce_prometheus_counter_for_presigned_urls

…s config

…om travis config" This reverts commit af42e0f.

williamhaley · 2021-05-14T20:35:23Z

fence/__init__.py

+from werkzeug.middleware.dispatcher import DispatcherMiddleware
+
+# This MUST be declared before the multiprocess lib is imported/initialized
+# to unblock unit testing without having to explicitly declare the env. variable
+# More details on this awkwardness: https://github.com/prometheus/client_python/issues/250
+tmp_dir = tempfile.TemporaryDirectory()
+os.environ["prometheus_multiproc_dir"] = tmp_dir.name
+
+from prometheus_client import CollectorRegistry, multiprocess, make_wsgi_app
+from prometheus_flask_exporter.multiprocess import UWsgiPrometheusMetrics


Is this our standard process? We load prometheus in the init and wire it up below? I haven't seen how we do this yet so wasn't sure if this is a new endeavor or just repeating a common pattern.

It is a new endeavor.
As far I have researched, this is the best way to accommodate a single point of configuration to track metrics around our RESTful API endpoints across all the UWSGI workers.

I haven't used prometheus at all, let alone at our org, but rather than coupling it with the code, could it trigger by watching the nginx logs?

https://docs.nginx.com/nginx-ingress-controller/logging-and-monitoring/prometheus/

I think this kind of solution might be nice in that it's agnostic from the code, and could be used to emit metrics for multiple types of services rather than just fence.

I took care of the nginx part already with some prometheus-exporter side card containers:
https://github.com/uc-cdis/cloud-automation/pull/1372/files

Now the next step is to introduce this new interface for us, so we can provide observability for our own functions.
(as in, not being agnostic from the code, but making observability blended into our code).
The presigned url is just the beginning.

imagine all the cool metrics we could observe to get better insights about our end-users' interactions:
https://github.com/uc-cdis/mfence/blob/master/src/mfence/blueprints/other_metrics.py#L16

williamhaley · 2021-05-14T20:36:10Z

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "fence"
-version = "4.22.0"
+version = "4.29.0"


Should this be 4.23.0? Not sure why this is bumping up to 4.29.0 from 4.22.0

Looks like the pyproject.toml versioning has not been updated.
Please check the current semantic versioning sequence in https://github.com/uc-cdis/fence/releases

👍 Thanks for updating that!

paulineribeyre · 2021-05-14T22:03:39Z

@themarcelor i cherry-picked the 3 commits into your branch as we discussed

…of github.com:uc-cdis/fence into chore/introduce_prometheus_counter_for_presigned_urls

…le id (guid) to avoid overloading prometheus

…of github.com:uc-cdis/fence into chore/introduce_prometheus_counter_for_presigned_urls

…f the file id (guid) to avoid overloading prometheus" This reverts commit db9af96.

paulineribeyre · 2021-05-17T19:58:13Z

fence/__init__.py

@@ -270,6 +279,10 @@ def app_config(
    _load_keys(app, root_dir)
    _set_authlib_cfgs(app)

+    app.prometheus_counters = {}
+    # TODO: if prometheus is disabled in config, do not setup


@themarcelor @williamhaley Prometheus can be enabled by default, but should we add a setting to the fence config file? would the ability to disable it be useful for Gen3 systems which are not managed by us, or maybe compose-services?

Why would we ever want to disable observability? 🤷🏼‍♂️

We’re not sending metrics to prometheus, Prometheus fetches the metrics, so the absence of prometheus is completely harmless

fence/blueprints/data/indexd.py

williamhaley

👍

task(Observability): Introduce prometheus-flask-exporter and enable M…

5e22bad

…onitoring for PreSigned URLs

set default /tmp path for prometheus_multiproc_dir when it is not def…

8b5ccf2

…ined

themarcelor added test-portal-dataUploadTest test-google-googleServiceAccountKeyTest labels Jan 21, 2021

themarcelor requested review from paulineribeyre, frickjack, itsJiaqi, m0nhawk, Avantol13 and BinamB January 21, 2021 15:40

themarcelor added jenkins-brain test-portal-homepageTest and removed jenkins-brain test-portal-dataUploadTest test-portal-homepageTest labels Jan 21, 2021

paulineribeyre reviewed Jan 22, 2021

View reviewed changes

themarcelor removed the test-google-googleServiceAccountKeyTest label Jan 23, 2021

themarcelor and others added 2 commits January 23, 2021 01:12

Merge branch 'master' into chore/introduce_prometheus_counter_for_pre…

c5f15f1

…signed_urls

Update tests/data/test_data.py

f4ceea5

Co-authored-by: Pauline Ribeyre <ribeyre@uchicago.edu>

paulineribeyre reviewed Jan 25, 2021

View reviewed changes

fence/blueprints/data/indexd.py Outdated Show resolved Hide resolved

themarcelor and others added 5 commits January 26, 2021 07:02

refactoring based on PR comments

72e2da5

Merge branch 'chore/introduce_prometheus_counter_for_presigned_urls' …

bc0bdc8

…of github.com:uc-cdis/fence into chore/introduce_prometheus_counter_for_presigned_urls

re-add prometheus_multiproc_dir environment variables for travis tests

d1293e4

moving counter declaration to a point after the imports

e8c5658

remove username pii and update branch with latest stuff

9fe9b54

paulineribeyre requested review from paulineribeyre and removed request for frickjack and itsJiaqi May 12, 2021 19:50

themarcelor and others added 11 commits May 13, 2021 17:16

set proper version

b83d5ba

address issues pointed out by codacity

6b245d1

set temporary directory for CollectorRegistry

114e1bf

remove prometheus metrics temp folder environment variable from travi…

af42e0f

…s config

Revert "remove prometheus metrics temp folder environment variable fr…

86080e1

…om travis config" This reverts commit af42e0f.

use PROMETHEUS_MULTIPROC_DIR env var

5240c7a

declare temp folder for unit tests

fd0f219

fix codacity issue

809f4dd

adding comments

8917cb6

Refactor prometheus initialization + fix import issues

8dd28eb

Split prometheus test

e21ff51

williamhaley reviewed May 14, 2021

View reviewed changes

paulineribeyre added 4 commits May 14, 2021 16:03

counter dict

b657510

Refactor prometheus initialization + fix import issues

ffdc593

Split prometheus test

4289eef

counter dict

4e8c737

themarcelor and others added 5 commits May 17, 2021 11:04

Merge branch 'chore/introduce_prometheus_counter_for_presigned_urls' …

1f21154

…of github.com:uc-cdis/fence into chore/introduce_prometheus_counter_for_presigned_urls

rename prometheus test

7971465

use a label to track the indexed file resource path instead of the fi…

db9af96

…le id (guid) to avoid overloading prometheus

Merge branch 'chore/introduce_prometheus_counter_for_presigned_urls' …

805d660

…of github.com:uc-cdis/fence into chore/introduce_prometheus_counter_for_presigned_urls

Revert "use a label to track the indexed file resource path instead o…

ad27a97

…f the file id (guid) to avoid overloading prometheus" This reverts commit db9af96.

paulineribeyre reviewed May 17, 2021

View reviewed changes

remove guid and leave just the protocol label

c51f3df

themarcelor added the test-portal-homepageTest label May 17, 2021

paulineribeyre approved these changes May 18, 2021

View reviewed changes

themarcelor merged commit e5d6947 into master May 18, 2021

themarcelor deleted the chore/introduce_prometheus_counter_for_presigned_urls branch May 18, 2021 16:13

williamhaley reviewed May 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

task(Observability): Introduce prometheus-flask-exporter and enable monitoring for presigned urls #864

task(Observability): Introduce prometheus-flask-exporter and enable monitoring for presigned urls #864

themarcelor commented Jan 21, 2021 •

edited

github-actions bot commented Jan 21, 2021

coveralls commented Jan 21, 2021 •

edited

paulineribeyre Jan 22, 2021

themarcelor Jan 25, 2021

williamhaley May 17, 2021

themarcelor May 18, 2021

williamhaley May 14, 2021

themarcelor May 14, 2021

williamhaley May 14, 2021 •

edited

themarcelor May 14, 2021 •

edited

williamhaley May 14, 2021

themarcelor May 14, 2021

williamhaley May 14, 2021

paulineribeyre commented May 14, 2021

paulineribeyre May 17, 2021 •

edited

themarcelor May 17, 2021 •

edited

paulineribeyre May 18, 2021

williamhaley left a comment

task(Observability): Introduce prometheus-flask-exporter and enable monitoring for presigned urls #864

task(Observability): Introduce prometheus-flask-exporter and enable monitoring for presigned urls #864

Conversation

themarcelor commented Jan 21, 2021 • edited

New Features

github-actions bot commented Jan 21, 2021

coveralls commented Jan 21, 2021 • edited

Pull Request Test Coverage Report for Build 10942

💛 - Coveralls

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

williamhaley May 14, 2021 • edited

Choose a reason for hiding this comment

themarcelor May 14, 2021 • edited

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

paulineribeyre commented May 14, 2021

paulineribeyre May 17, 2021 • edited

Choose a reason for hiding this comment

themarcelor May 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamhaley left a comment

Choose a reason for hiding this comment

themarcelor commented Jan 21, 2021 •

edited

coveralls commented Jan 21, 2021 •

edited

williamhaley May 14, 2021 •

edited

themarcelor May 14, 2021 •

edited

paulineribeyre May 17, 2021 •

edited

themarcelor May 17, 2021 •

edited