Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add group_unhandled_paths Option to Enhance filter_unhandled_paths Functionality #91 #98

Merged

Conversation

piotrhyzy
Copy link

PR Title: Add group_unhandled_paths Option to Enhance filter_unhandled_paths Functionality

Hello @stephenhillier and @Filipoliko,

Following the discussion in the previous PR and the suggestions made, I have created an alternative PR (based on @Filipoliko solution) for handling unhandled paths.

Overview of the Changes:

  • Introduced the group_unhandled_paths option.
  • group_unhandled_paths: Similar to filter_unhandled_paths, but instead of ignoring the requests, they are grouped under the __unknown__ path. This option overrides filter_unhandled_paths by setting it to False. The default value is False.

Reasoning:

  • This approach simplifies configuration by linking the two options, reducing potential user confusion.
  • Ensures clear intent when configuring the handling of unhandled paths.
  • Aligns with the feedback suggesting that group_unhandled_paths provides clear intend even if filter_unhandled_paths is enabled and simply it overrides it.

I believe this solution addresses the concerns raised about having mutually exclusive options and makes the configuration more intuitive.

Please review this alternative approach and let me know your thoughts.

Copy link
Owner

@stephenhillier stephenhillier left a comment

Choose a reason for hiding this comment

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

Looks great. I agree with your comments on the two options, having group_unhandled_paths override makes sense to me.

One thing I would say is that users who only set group_unhandled_paths will see the warning, even if they didn't intentionally set filter_unhandled_paths (since it defaults to True). That's a minor issue and we can always adjust the warning later if needed.

starlette_exporter/middleware.py Outdated Show resolved Hide resolved
starlette_exporter/middleware.py Outdated Show resolved Hide resolved
@piotrhyzy piotrhyzy force-pushed the feature/group_unhandled_paths branch from f32b57a to 37d3b09 Compare June 22, 2024 09:08
@piotrhyzy piotrhyzy marked this pull request as ready for review June 22, 2024 09:12
@stephenhillier
Copy link
Owner

Thanks for the update, looks good.

@stephenhillier stephenhillier merged commit ef3a18b into stephenhillier:master Jun 23, 2024
7 checks passed
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.

None yet

3 participants