Merge branch 'chore/introduce_prometheus_counter_for_presigned_urls' …

…of github.com:uc-cdis/fence into pauline/test

Dockerfile

@@ -1,7 +1,7 @@
 # To run: docker run --rm -d -v /path/to/fence-config.yaml:/var/www/fence/fence-config.yaml --name=fence -p 80:80 fence
 # To check running container: docker exec -it fence /bin/bash
 
-FROM quay.io/cdis/python-nginx:pybase3-1.4.2
+FROM quay.io/cdis/python-nginx:chore_scrape_metrics_from_services
 
 ENV appname=fence
 

clear_prometheus_multiproc

@@ -1,4 +1,6 @@
 #!/bin/bash
+# This script is immediatelly executed by uwsgi during startup
+# it prepares the prometheus_multiproc_dir folder to store the metrics from separate uwsgi workers (per PID)
 set -ex
 
 rm -Rf $1

deployment/uwsgi/uwsgi.ini

@@ -23,12 +23,14 @@ vacuum = true
 pythonpath = /var/www/fence/
 pythonpath = /fence/
 pythonpath = /usr/local/lib/python3.6/site-packages/
+# poetry installs git dependencies at /usr/local/src
+pythonpath = /usr/local/src/*
+
+# metrics setup
 stats = 127.0.0.1:9191
 stats-http = true
 env = prometheus_multiproc_dir=/var/tmp/uwsgi_flask_metrics
 exec-asap = /fence/clear_prometheus_multiproc /var/tmp/uwsgi_flask_metrics
-# poetry installs git dependencies at /usr/local/src
-pythonpath = /usr/local/src/*
 
 # Initialize application in worker processes, not master. This prevents the
 # workers from all trying to open the same database connections at startup.

dockerrun.bash

@@ -18,10 +18,3 @@ if [ -f /fence/jwt-keys.tar ]; then
     fi
   )
 fi
-
-(
-  while true; do
-    curl -s http://127.0.0.1/metrics >> /var/www/metrics/metrics.txt
-    sleep 12
-  done
-) &

fence/__init__.py

@@ -6,7 +6,6 @@
 from flask_cors import CORS
 from flask_sqlalchemy_session import flask_scoped_session, current_session
 from userdatamodel.driver import SQLAlchemyDriver
-
 from werkzeug.middleware.dispatcher import DispatcherMiddleware
 from prometheus_client import CollectorRegistry, multiprocess, make_wsgi_app
 from prometheus_flask_exporter.multiprocess import UWsgiPrometheusMetrics
@@ -56,17 +55,9 @@
 # Later, in app_config(), will actually set level based on config
 logger = get_logger(__name__, log_level="debug")
 
-if "prometheus_multiproc_dir" not in os.environ:
-    os.environ["prometheus_multiproc_dir"] = "/tmp"
-
-registry = CollectorRegistry()
-multiprocess.MultiProcessCollector(registry)
-
 app = flask.Flask(__name__)
 CORS(app=app, headers=["content-type", "accept"], expose_headers="*")
 
-metrics = UWsgiPrometheusMetrics(app)
-
 
 def warn_about_logger():
     raise Exception(
@@ -96,6 +87,23 @@ def app_init(
     server.init_app(app, query_client=query_client)
 
 
+def setup_prometheus(app):
+    if "prometheus_multiproc_dir" not in os.environ:
+        os.environ["prometheus_multiproc_dir"] = "/tmp"
+
+    registry = CollectorRegistry()
+    multiprocess.MultiProcessCollector(registry)
+
+    metrics = UWsgiPrometheusMetrics(app)
+
+    # Add prometheus wsgi middleware to route /metrics requests
+    app.wsgi_app = DispatcherMiddleware(
+        app.wsgi_app, {"/metrics": make_wsgi_app(registry=registry)}
+    )
+
+    return registry
+
+
 def app_sessions(app):
     app.url_map.strict_slashes = False
     app.db = SQLAlchemyDriver(config["DB"])
@@ -431,7 +439,4 @@ def set_csrf(response):
     return response
 
 
-# Add prometheus wsgi middleware to route /metrics requests
-app.wsgi_app = DispatcherMiddleware(
-    app.wsgi_app, {"/metrics": make_wsgi_app(registry=registry)}
-)
+registry = setup_prometheus(app)

fence/blueprints/data/indexd.py

@@ -9,16 +9,8 @@
 from cdispyutils.config import get_value
 from cdispyutils.hmac4 import generate_aws_presigned_url
 import flask
-import requests
 from prometheus_flask_exporter import Counter
-
-# from fence.user import get_current_user
-
-pre_signed_url_req = Counter(
-    "pre_signed_url_req",
-    "tracking presigned url requests",
-    ["username", "file_id", "requested_protocol"],
-)
+import requests
 
 from fence.auth import (
     get_jwt,
@@ -60,6 +52,13 @@
 ANONYMOUS_USER_ID = "anonymous"
 ANONYMOUS_USERNAME = "anonymous"
 
+# gen3 metrics
+pre_signed_url_req = Counter(
+    "pre_signed_url_req",
+    "tracking presigned url requests",
+    ["username", "guid", "requested_protocol"],
+)
+
 
 def get_signed_url_for_file(action, file_id, file_name=None):
     requested_protocol = flask.request.args.get("protocol", None)

fence/scripting/fence_create.py

@@ -543,6 +543,7 @@ def remove_expired_google_service_account_keys(db):
 
             # handle service accounts with custom expiration
             for expired_user_key in expired_sa_keys_for_users:
+                logger.info("expired_user_key: {}\n".format(expired_user_key))
                 sa = (
                     current_session.query(GoogleServiceAccount)
                     .filter(
@@ -555,6 +556,9 @@ def remove_expired_google_service_account_keys(db):
                     account=sa.email, key_name=expired_user_key.key_id
                 )
                 response_error_code = response.get("error", {}).get("code")
+                response_error_status = response.get("error", {}).get("status")
+                logger.info("response_error_code: {}\n".format(response_error_code))
+                logger.info("response_error_status: {}\n".format(response_error_status))
 
                 if not response_error_code:
                     current_session.delete(expired_user_key)
@@ -564,7 +568,10 @@ def remove_expired_google_service_account_keys(db):
                             expired_user_key.key_id, sa.email, sa.user_id
                         )
                     )
-                elif response_error_code == 404:
+                elif (
+                    response_error_code == 404
+                    or response_error_status == "FAILED_PRECONDITION"
+                ):
                     logger.info(
                         "INFO: Service account key {} for service account {} "
                         "(owned by user with id {}) does not exist in Google. "

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "fence"
-version = "4.22.1"
+version = "4.26.0"
 description = "Gen3 AuthN/AuthZ OIDC Service"
 authors = ["CTDS UChicago <cdis@uchicago.edu>"]
 license = "Apache-2.0"
@@ -34,6 +34,8 @@ gen3users = "^0.6.0"
 idna = "^2.10" # https://github.com/python-poetry/poetry/issues/3555
 markdown = "^3.1.1"
 paramiko = "^2.6.0"
+prometheus-client = "^0.9.0"
+prometheus-flask-exporter = "^0.18.1"
 psycopg2 = "^2.8.3"
 pyjwt = "^1.5.3"
 python_dateutil = "^2.6.1"
@@ -45,8 +47,6 @@ sqlalchemy = "^1.3.3"
 storageclient = {git = "https://github.com/uc-cdis/storage-client", rev = "1.0.1"}
 userdatamodel = "^2.3.3"
 werkzeug = "^0.16.0"
-prometheus-client = "^0.9.0"
-prometheus-flask-exporter = "^0.18.1"
 
 [tool.poetry.dev-dependencies]
 addict = "^2.2.1"

tests/data/test_data.py

@@ -10,6 +10,7 @@
 import fence.blueprints.data.indexd
 from fence.config import config
 from fence.errors import NotSupported
+from fence import registry
 
 from tests import utils
 
@@ -18,8 +19,6 @@
 import cirrus
 from cirrus import GoogleCloudManager
 
-from fence import registry
-
 
 @pytest.mark.parametrize(
     "indexd_client", ["gs", "s3", "gs_acl", "s3_acl", "s3_external"], indirect=True
@@ -44,7 +43,7 @@ def test_indexd_download_file(
             "pre_signed_url_req_total",
             {
                 "username": "test",
-                "file_id": "1",
+                "guid": "1",
                 "requested_protocol": indexd_client["indexed_file_location"],
             },
         )
@@ -74,11 +73,11 @@ def test_indexd_download_file(
         "pre_signed_url_req_total",
         {
             "username": "test",
-            "file_id": "1",
+            "guid": "1",
             "requested_protocol": indexd_client["indexed_file_location"],
         },
     )
-    assert 1 == (after - before)
+    assert 1 == (after - before), "1 presigned URL request should have been counted"
 
     # defaults to signing url, check that it's not just raw url
     assert urllib.parse.urlparse(response.json["url"]).query != ""