Skip to content

Conversation

@nss10
Copy link
Contributor

@nss10 nss10 commented Aug 15, 2025

Link to JIRA ticket if there is one: MIDRC-1098

This PR fixes an issue where requests to the /metrics endpoint were incorrectly falling back to s3_root_router.

  • Updates routing order so /metrics is handled correctly by the metrics handler.
  • Adds a unit test to verify /metrics responds as expected, preventing regressions.

NOTE: This PR depends on -- uc-cdis/cdis-python-utils#72

Bug Fixes

  • Fix: /metrics endpoint will be correctly routed to prometheus app, previously it was accidentally being routed to s3_root_router

@github-actions
Copy link

The style in this PR agrees with black. ✔️

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

@coveralls
Copy link

coveralls commented Aug 15, 2025

Pull Request Test Coverage Report for Build 17554658438

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 87.634%

Files with Coverage Reduction New Missed Lines %
app.py 2 93.75%
Totals Coverage Status
Change from base Build 16608366178: 0.2%
Covered Lines: 574
Relevant Lines: 655

💛 - Coveralls

nss10 added a commit that referenced this pull request Aug 15, 2025
@nss10 nss10 requested a review from Avantol13 August 15, 2025 18:27
@nss10
Copy link
Contributor Author

nss10 commented Aug 28, 2025

Waiting for -- uc-cdis/cdis-python-utils#72 to be reviewed and merged

Copy link

@markxiao markxiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code look good. I did not test myself b/c my env is not up yet.

@nss10 nss10 merged commit cf4ddb5 into master Sep 8, 2025
8 checks passed
@nss10 nss10 deleted the chore/fix_metrics_endpoint branch October 6, 2025 22:19
if app.metrics.enabled:
app.mount("/metrics", app.metrics.get_asgi_app())

app.include_router(s3_root_router, tags=["S3"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nss10 When you have a chance could you add a comment explaining why it's not done at the same time as the other routers 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants