Skip to content

Conversation

rads-1996
Copy link
Contributor

@rads-1996 rads-1996 commented Sep 29, 2025

Fixed an issue #2352 where http_server_request_duration metrics was being recorded for excluded urls.

Description

Fixes #2352

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • A flask application was set up using the reproduction steps outlined in the linked issue, and the before-and-after changes have been captured and attached to the PR.
  • Modified existing unit tests to conform to new functionality

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rads-1996 rads-1996 requested a review from a team as a code owner September 29, 2025 21:26
@rads-1996
Copy link
Contributor Author

In the current scenario, when requests are made to the excluded /health endpoint, each hit is recorded as a failed request. For this instance, 7 requests were made to the /health endpoint -

image

After the change, server request duration metrics are recorded only for non-excluded URLs. In this case, 3 requests were made to the /health endpoint -

Screenshot 2025-09-29 143914

@rads-1996 rads-1996 marked this pull request as draft September 29, 2025 21:59
@rads-1996 rads-1996 marked this pull request as ready for review September 29, 2025 23:35
@xrmx xrmx moved this to Ready for review in @xrmx's Python PR digest Sep 30, 2025
@xrmx xrmx changed the title Fixed issue #2352 flask: fixed http_server_request_duration metrics being recorded for excluded urls Oct 10, 2025
@xrmx xrmx moved this from Ready for review to Reviewed PRs that need fixes in @xrmx's Python PR digest Oct 10, 2025
attr,
_recommended_metrics_attrs_old[metric.name],
)
self.assertTrue(number_data_point_seen and histogram_data_point_seen)
Copy link
Member

Choose a reason for hiding this comment

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

histogram_data_point_seen should be false no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the histogram_data_point is false for the excluded_urls but true for the non-excluded urls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is confusing, I can simply remove this test as the test below this one test_flask_metrics_new_semconv_excluded_urls, is doing pretty much the same thing but for all three URLs.

Copy link
Member

@emdneto emdneto Oct 15, 2025

Choose a reason for hiding this comment

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

I would suggest you keep both, but instead make them identical in terms of testing. For example: test_flask_metrics_excluded_urls and test_flask_metrics_excluded_urls_new_semconv (the suffix here is important). They should be the same test, but asserting different attributes from semconv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emdneto Fixed the test names.

Copy link
Member

@emdneto emdneto Oct 15, 2025

Choose a reason for hiding this comment

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

@rads-1996 can you make both tests identical? either having the call to /hello/756 or not, or create a new test if you want a mix of excluded urls + allowed urls

Copy link
Contributor Author

@rads-1996 rads-1996 Oct 15, 2025

Choose a reason for hiding this comment

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

@emdneto I made the assertions in both the tests same. This tests only the excluded urls.

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

Labels

None yet

Projects

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

Azure Exclude URL Errors

3 participants