feat: add collapse_request_paths_depth config option#1273
Open
kalEl2001 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Prometheus configuration option ChangesPrometheus Request Path Collapsing Depth
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.schema/config.schema.json (1)
1260-1264: ⚡ Quick winEnforce a positive lower bound for
collapse_request_paths_depth.The description says this is only meaningful for values greater than
0, but the schema currently accepts0and negatives. Add a lower bound to reject invalid config early.Suggested patch
"collapse_request_paths_depth": { "type": "integer", + "minimum": 1, "default": 1, "title": "CollapsePathsDepth", "description": "When set to a value greater than 0 the request label will include the first N segments of the request path" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.schema/config.schema.json around lines 1260 - 1264, The schema property "collapse_request_paths_depth" currently allows 0 and negative integers despite the description requiring values > 0; add a validation lower bound by adding "minimum": 1 to the "collapse_request_paths_depth" property in the JSON schema so configs with 0 or negatives are rejected early (keep the existing "type", "default", "title", and "description" intact).spec/config.schema.json (1)
1260-1264: ⚡ Quick winMirror the positive lower bound for
collapse_request_paths_depthhere as well.This schema copy should also enforce
>= 1so invalid values are rejected consistently.Suggested patch
"collapse_request_paths_depth": { "type": "integer", + "minimum": 1, "default": 1, "title": "CollapsePathsDepth", "description": "When set to a value greater than 0 the request label will include the first N segments of the request path" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/config.schema.json` around lines 1260 - 1264, The JSON schema property "collapse_request_paths_depth" currently has "type", "default", "title", and "description" but lacks a lower bound; add a "minimum": 1 to that property so the schema enforces values >= 1 (mirror the lower bound used elsewhere) ensuring invalid values are rejected consistently for "collapse_request_paths_depth".driver/configuration/provider_koanf_public_test.go (1)
167-172: ⚡ Quick winAdd one non-default override case for the new getter.
Current coverage validates only the default value from fixture config. Add a direct override assertion to verify the accessor wiring for configured values.
Suggested test addition
t.Run("group=prometheus", func(t *testing.T) { assert.Equal(t, "localhost:9000", p.PrometheusServeAddress()) assert.Equal(t, "/metrics", p.PrometheusMetricsPath()) assert.Equal(t, true, p.PrometheusCollapseRequestPaths()) assert.Equal(t, 1, p.PrometheusCollapseRequestPathsDepth()) + + p.SetForTest(t, configuration.PrometheusServeCollapseRequestPathsDepth, 3) + assert.Equal(t, 3, p.PrometheusCollapseRequestPathsDepth()) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@driver/configuration/provider_koanf_public_test.go` around lines 167 - 172, Add a new subtest in provider_koanf_public_test.go that sets non-default values and asserts the new getters return those overrides: call the provider (p) with an explicit override config for PrometheusServeAddress, PrometheusMetricsPath, PrometheusCollapseRequestPaths, and PrometheusCollapseRequestPathsDepth and assert the returned values from PrometheusServeAddress(), PrometheusMetricsPath(), PrometheusCollapseRequestPaths(), and PrometheusCollapseRequestPathsDepth() match the overridden inputs to verify accessor wiring for configured values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metrics/middleware.go`:
- Around line 76-80: CollapsePaths accepts any depth and writes without
validating, and ServeHTTP reads shared fields without using RLock; validate the
depth in CollapsePaths (e.g., ensure depth > 0 and clamp or set a sane default)
before acquiring the write lock and setting m.collapsePathsDepth, and in
ServeHTTP wrap all reads of m.silencePaths, m.hidePaths, m.collapsePaths, and
m.collapsePathsDepth with m.mutex.RLock()/RUnlock() to avoid concurrent-access
races (also apply the same RLock protection to the other read sites mentioned
around ServeHTTP and the blocks at the ranges noted).
---
Nitpick comments:
In @.schema/config.schema.json:
- Around line 1260-1264: The schema property "collapse_request_paths_depth"
currently allows 0 and negative integers despite the description requiring
values > 0; add a validation lower bound by adding "minimum": 1 to the
"collapse_request_paths_depth" property in the JSON schema so configs with 0 or
negatives are rejected early (keep the existing "type", "default", "title", and
"description" intact).
In `@driver/configuration/provider_koanf_public_test.go`:
- Around line 167-172: Add a new subtest in provider_koanf_public_test.go that
sets non-default values and asserts the new getters return those overrides: call
the provider (p) with an explicit override config for PrometheusServeAddress,
PrometheusMetricsPath, PrometheusCollapseRequestPaths, and
PrometheusCollapseRequestPathsDepth and assert the returned values from
PrometheusServeAddress(), PrometheusMetricsPath(),
PrometheusCollapseRequestPaths(), and PrometheusCollapseRequestPathsDepth()
match the overridden inputs to verify accessor wiring for configured values.
In `@spec/config.schema.json`:
- Around line 1260-1264: The JSON schema property "collapse_request_paths_depth"
currently has "type", "default", "title", and "description" but lacks a lower
bound; add a "minimum": 1 to that property so the schema enforces values >= 1
(mirror the lower bound used elsewhere) ensuring invalid values are rejected
consistently for "collapse_request_paths_depth".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b433253-52b4-48c8-9aa0-352399081376
📒 Files selected for processing (10)
.schema/config.schema.jsoncmd/server/server.godriver/configuration/config_keys.godriver/configuration/provider.godriver/configuration/provider_koanf.godriver/configuration/provider_koanf_public_test.gointernal/config/.oathkeeper.yamlmetrics/middleware.gometrics/middleware_test.gospec/config.schema.json
kalEl2001
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue(s)
#1212
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.com) from the maintainers to push
the changes.
works.
Further Comments
Add
collapse_request_paths_depthconfiguration to control how many pathsegments are included in Prometheus metrics labels.
Previously,
collapse_request_paths: truewould always collapse to just thefirst path segment (e.g.
/decisions/service/foo→/decisions). This changeintroduces
collapse_request_paths_depth(default:1) so operators caninclude more segments without exposing full dynamic paths
(e.g. with depth
2:/decisions/service/foo→/decisions/service).I also have created a PR in the docs repo. Please check this PR.
Summary by CodeRabbit
collapse_request_paths_depthfor Prometheus metrics, letting you control how many leading path segments are kept when request paths are collapsed.1; the setting applies to served metrics so collapsed request labels include the first N path segments.