chore(metrics): init Metrics in app init, cleanup setup code to one b… #1167

Avantol13 · 2024-07-26T15:52:36Z

…ash script, begin migration of common code to library

New Features

Breaking Changes

Bug Fixes

Improvements

Cleanup and move some of the new metrics logic using prometheus to the cdispyutils library

Dependency updates

Deployment changes

…ash script, begin migration of common code to library

coveralls · 2024-07-26T15:56:13Z

Pull Request Test Coverage Report for Build 10201086602

Details

0 of 0 changed or added relevant lines in 0 files are covered.
54 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.002%) to 75.143%

Files with Coverage Reduction	New Missed Lines	%
resources/ga4gh/passports.py	2	87.43%
metrics.py	3	94.0%
blueprints/login/base.py	9	82.28%
init.py	17	90.84%
blueprints/data/indexd.py	23	94.59%

Totals
Change from base Build 10164514863:	0.002%
Covered Lines:	7751
Relevant Lines:	10315

💛 - Coveralls

paulineribeyre · 2024-07-26T20:52:02Z

.github/workflows/ci.yaml

+# push will run on every pushed commit to any branch (so this will rerun the tests
+# once a branch gets merged to main in addition to any new commits on any branch)
+on: push
+
 name: CI
-on:
-  push:
-  pull_request:
-    types: [opened, reopened]


There can be edge cases where the "automatic merge" messes something up, and the CI succeeds on the "branch run", but fails on the "PR run". In that case you have to update your branch manually and fix the conflict that git didn't see. It has happened to me a handful of times, not sure it's worth running the CI twice on every PR, but 🤷‍♀️

Does it happen in other repos? which automatic merge are you referring to?

it can happen in any repo

i can't find documentation about it but this is my understanding from what i observed:
when 2 checks run (for push and for PR) they're not necessarily running on the same code. The "push" check runs on the branch's code, and the "PR" check runs on the code that would be on master after merging the PR. If there are any changes on master that aren't on the branch, it performs a merge automatically and runs the CI on that. It's the same code that would end up on master if you choose to merge without updating your branch first - git/github finds no conflict and merges. Occasionally it does it wrong, and it can be caught by the "PR" CI check

We should be protected against people merging without first updating their branch, at which point the push should run again. I do remember issues in the past vaguely... but I can't find any documentation in GH Actions that leads me to believe it's necessary to have both of these. Unless you have a strong opinion, I would rather not run everything twice and just get feedback on the code on main when the branch-creator merges it in and pushes those commits.

fence/__init__.py

fence/metrics.py

tests/ci_commands_script.sh

paulineribeyre · 2024-07-26T20:57:22Z

tests/test_metrics.py

+@pytest.fixture(scope="function")
+def disable_metrics_app(app):
+    """
+    temporarily disable metrics on the session-scoped app for this function


i'm wondering if we should do the opposite... 🤔 set ENABLE_PROMETHEUS_METRICS to false in the tests by default and only enable metrics on the tests that need them?

well, I think it's more likely that we'll have this on for our deployments, so I actually like that it's on for unrelated tests, making sure whatever we add for metrics doesn't accidentally break something existing

Co-authored-by: Pauline Ribeyre <[email protected]>

fence/metrics.py

pyproject.toml

…e/metrics-cleanup

paulineribeyre · 2024-08-01T20:05:42Z

Dockerfile

@@ -41,7 +41,7 @@ RUN poetry config virtualenvs.create false \
 COPY . /$appname
 COPY ./deployment/uwsgi/uwsgi.ini /etc/uwsgi/uwsgi.ini
 COPY ./deployment/uwsgi/wsgi.py /$appname/wsgi.py
-COPY clear_prometheus_multiproc /$appname/clear_prometheus_multiproc
+COPY ./deployment/scripts/metrics/setup_prometheus /$appname/setup_prometheus


i think we can delete this file now https://github.com/uc-cdis/fence/blob/7b0aa60/clear_prometheus_multiproc

paulineribeyre · 2024-08-01T20:08:50Z

fence/metrics.py

+                            doing nothing. This is the behavior when metrics are disabled. Why? So application code
+                            doesn't have to check, it always tries to log a metric.
+            prometheus_dir (str): Directory to use when setting PROMETHEUS_MULTIPROC_DIR env var (which prometheus requires
+                                  for multiprocess metrics collection). Note that this the prometheus client is very


Suggested change

for multiprocess metrics collection). Note that this the prometheus client is very

for multiprocess metrics collection). Note that the prometheus client is very

chore(metrics): init Metrics in app init, cleanup setup code to one b…

0a8556b

…ash script, begin migration of common code to library

github-actions bot added the @requires-fence label Jul 26, 2024

chore(ci): don't run every check twice on prs

e04afeb

paulineribeyre reviewed Jul 26, 2024

View reviewed changes

Avantol13 and others added 3 commits July 29, 2024 08:41

Update fence/__init__.py

986b93e

Co-authored-by: Pauline Ribeyre <[email protected]>

chore(refactor): non-flask specific directory, address review comments

f1279cc

chore(docs): add details about running unit tests

f47ef83

paulineribeyre previously approved these changes Jul 29, 2024

View reviewed changes

fence/metrics.py Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

chore(lock): use updated library

5f80e58

Avantol13 dismissed paulineribeyre’s stale review via 5f80e58 July 30, 2024 13:15

Avantol13 and others added 7 commits July 30, 2024 10:43

Merge branch 'master' into chore/metrics-cleanup

6275a44

chore(links): correct microsoft links so they resolve

3b9fc15

Merge remote-tracking branch 'origin/chore/metrics-cleanup' into chor…

7b8a1b1

…e/metrics-cleanup

fix(links): use new microsoft learning links to docs

a41aa20

chore(docs): update docstring

f5a0304

fix(links): fix broken docs links

868fdf2

fix(metrics): fix path to setup prometheus file

97a24f5

Avantol13 added test-apis-dbgapTest test-apis-centralizedAuth and removed test-apis-dbgapTest labels Jul 31, 2024

chore(deps): relock, new version

68abf8c

paulineribeyre approved these changes Aug 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(metrics): init Metrics in app init, cleanup setup code to one b… #1167

chore(metrics): init Metrics in app init, cleanup setup code to one b… #1167

Avantol13 commented Jul 26, 2024 •

edited

Loading

coveralls commented Jul 26, 2024 •

edited

Loading

paulineribeyre Jul 26, 2024

Avantol13 Jul 29, 2024

paulineribeyre Jul 29, 2024

Avantol13 Jul 29, 2024

paulineribeyre Jul 26, 2024

Avantol13 Jul 29, 2024

paulineribeyre Aug 1, 2024

paulineribeyre Aug 1, 2024

	for multiprocess metrics collection). Note that this the prometheus client is very
	for multiprocess metrics collection). Note that the prometheus client is very

chore(metrics): init Metrics in app init, cleanup setup code to one b… #1167

Are you sure you want to change the base?

chore(metrics): init Metrics in app init, cleanup setup code to one b… #1167

Conversation

Avantol13 commented Jul 26, 2024 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

coveralls commented Jul 26, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10201086602

Details

💛 - 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avantol13 commented Jul 26, 2024 •

edited

Loading

coveralls commented Jul 26, 2024 •

edited

Loading