Fix continuous profiling policy validation: reject zero threshold/count#13898
Open
mrproliu wants to merge 2 commits into
Open
Fix continuous profiling policy validation: reject zero threshold/count#13898mrproliu wants to merge 2 commits into
mrproliu wants to merge 2 commits into
Conversation
…ount The validation conditions used `< 0` while the error messages said "must bigger than zero". rover triggers profiling when `value >= threshold` (and `matchedCount >= count`), so a threshold/count of 0 always fires and is meaningless. Tighten the conditions to `<= 0`, align CPU percent / HTTP error rate to (0-100], and fix the message grammar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes continuous profiling policy validation in OAP so that thresholds/counts of 0 are rejected, aligning the validation logic with the existing error-message semantics and preventing “always-trigger” policies. It also updates the change log to document the behavioral tightening.
Changes:
- Tighten validation for continuous profiling thresholds/counts to reject
0(e.g.,<= 0instead of< 0) and narrow percent-based thresholds from[0-100]to(0-100]. - Improve several validation error messages’ grammar (e.g., “must be bigger than zero”, “smaller than”).
- Add a CHANGES entry documenting the new rejection behavior and the percent-range tightening.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/continuous/ContinuousProfilingMutationService.java | Tightens policy item validation to reject zero thresholds/counts and adjusts related messages. |
| docs/en/changes/changes.md | Documents the validation tightening and the effect on threshold ranges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Fix [Bug] Continuous profiling policy validation: condition doesn't match error message (#13832)
In
ContinuousProfilingMutationService.validatePolicyItem()/validatePolicyItemWindows(), the validation conditions used< 0while the error messages said "must bigger than zero". The condition therefore accepted0, contradicting the message.0is not a meaningful threshold. In skywalking-rover the profiling trigger fires whenvalue >= threshold(system_checker.go,http_checker.go) and a time window matches whenmatchedCount >= count(windows.go). A threshold or count of0is always satisfied, i.e. it would trigger profiling unconditionally.Fix (Option A — fix the condition to match the message semantics):
PROCESS_THREAD_COUNT,SYSTEM_LOAD,HTTP_AVG_RESPONSE_TIME:< 0→<= 0.countwindow check:< 0→<= 0(same bug, not flagged in the issue).PROCESS_CPU,HTTP_ERROR_RATE: tightened from[0-100]to(0-100]for consistency (a0%threshold also always triggers).Fixed message grammar: "must bigger than zero" → "must be bigger than zero", "small than" → "smaller than".
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes [Bug] Continuous profiling policy validation: condition doesn't match error message #13832.
Update the `CHANGES` log.