feat(prometheus): add SGLang profile and independent rollup limits - #23861
Draft
ktsaou wants to merge 2 commits into
Draft
feat(prometheus): add SGLang profile and independent rollup limits#23861ktsaou wants to merge 2 commits into
ktsaou wants to merge 2 commits into
Conversation
github-actions
Bot
added
area/docs
area/collectors
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 17 files
Confidence score: 5/5
- In
src/go/internal/promprofile/validation/validate.go, max-time-series rejection diagnostics show an emptymetricvalue, which makes the warning less useful to users; populateMetricFamilyNameor omit the empty metric field.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/go/internal/promprofile/validation/validate.go">
<violation number="1" location="src/go/internal/promprofile/validation/validate.go:549">
P3: For a max_time_series (total-series) context rejection, the added validation warning always prints an empty metric="" because the emitted PlanRouteDiagnostic never sets MetricFamilyName for the series-cap reason. Omit the metric segment when it is empty, or only include it for the per-metric reason.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Collector as Prometheus Collector
participant Metrix as Metrix Store
participant Engine as Chart Engine
participant Planner as Plan Builder
participant Diary as Route Diagnostics
Note over Collector,Diary: Prometheus profile collection flow with per-context series limits
Collector->>Collector: check() probes endpoint
alt Profiled job selected
Collector->>Collector: Skip startup total series check
Collector->>Collector: Disable per-metric writer limit (policy.maxTSPerMetric = 0)
else No profile selected
Collector->>Collector: Apply max_time_series as startup check
Collector->>Collector: Retain writer-side per-metric limit
end
Collector->>Metrix: Scrape and retain full source population
Collector->>Metrix: Apply profile chart template
Collector->>Engine: EnginePolicy() - pass MaxTimeSeries & MaxTimeSeriesPerMetric
Engine->>Planner: PreparePlan()
Planner->>Planner: Scan and aggregate all source series
Planner->>Planner: Build per-chart observedCount and sourceDimensions
Note over Planner: enforceContextLimits() - NEW per-context admission
Planner->>Planner: Aggregate limits by chart context
alt Context exceeds max_time_series
Planner->>Planner: Mark context rejected (PlanRouteReasonContextSeriesCap)
else Metric family exceeds max_time_series_per_metric
Planner->>Planner: Mark context rejected (PlanRouteReasonContextMetricSeriesCap)
end
loop Each over-limit context
Planner->>Planner: Delete all charts in context
Planner->>Diary: Emit PlanRouteDiagnostic (rejected, context, series count)
end
Planner-->>Collector: Plan with admitted contexts only
alt Contexts admitted
Planner->>Collector: Complete chart rollups materialized
Collector-->>Graph: Render retained contexts
else Over-limit context skipped
Collector-->>Graph: Context omitted whole (retried next scrape)
end
Note over Collector,Diary: Retry behavior - same admission logic on each scrape cycle
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if fact.Reason == chartengine.PlanRouteReasonContextMetricSeriesCap { | ||
| setting = "max_time_series_per_metric" | ||
| } | ||
| r.addWarning("context_series_limit", context, |
Contributor
There was a problem hiding this comment.
P3: For a max_time_series (total-series) context rejection, the added validation warning always prints an empty metric="" because the emitted PlanRouteDiagnostic never sets MetricFamilyName for the series-cap reason. Omit the metric segment when it is empty, or only include it for the per-metric reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/go/internal/promprofile/validation/validate.go, line 549:
<comment>For a max_time_series (total-series) context rejection, the added validation warning always prints an empty metric="" because the emitted PlanRouteDiagnostic never sets MetricFamilyName for the series-cap reason. Omit the metric segment when it is empty, or only include it for the per-metric reason.</comment>
<file context>
@@ -539,6 +540,16 @@ func validateStep(parent context.Context, s *validationSession, dumpPath string)
+ if fact.Reason == chartengine.PlanRouteReasonContextMetricSeriesCap {
+ setting = "max_time_series_per_metric"
+ }
+ r.addWarning("context_series_limit", context,
+ fmt.Sprintf("context omitted: %s=%d, output series=%d, metric=%q", setting, fact.SeriesLimit, fact.SeriesCount, fact.MetricFamilyName),
+ "The complete context exceeds the existing limit. Other contexts are admitted independently; this context is retried each scrape.")
</file context>
ktsaou
changed the title
Contributor
There was a problem hiding this comment.
4 issues found across 18 files (changes from recent commits).
Confidence score: 3/5
src/go/internal/promprofile/semantics/normalizations.gocan mark output as optional even when exact rules and ranges cover the entire unsigned-integer domain, causing incorrect presence semantics; derive always-present status from domain coverage rather than theunknownfallback.src/go/internal/promprofile/semantics/units.gomay label an absolutedata_rategauge withbytes/s²when the unit rate isper_second; restrict the squaring logic to the applicable rate mode.src/go/plugin/go.d/collector/prometheus/profile-proofs/sglang/OPERATOR-MODEL.mdomits the header expected by sibling documents, which can fail markdownlint CI; add the standard header.src/go/internal/promprofile/semantics/replay_source.goreports an emptyValuesslice for invalid unsigned-integer labels, making replay failures less actionable; include the rejected value or canonical-domain expectation in the error.
Not reviewed (too large): src/go/plugin/go.d/collector/prometheus/profile-proofs/sglang/PROFILE-DESIGN.yaml (~5,690 lines), src/go/plugin/go.d/config/go.d/prometheus.profiles/default/sglang.yaml (~3,983 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/go/plugin/go.d/collector/prometheus/profile-proofs/sglang/OPERATOR-MODEL.md">
<violation number="1" location="src/go/plugin/go.d/collector/prometheus/profile-proofs/sglang/OPERATOR-MODEL.md:1">
P2: This new OPERATOR-MODEL.md omits the `` header that every sibling OPERATOR-MODEL.md in profile-proofs carries. Codacy runs markdownlint in CI (see .agents/skills/triage-codacy/how-tos/reproduce-pr-22423-markdownlint.md), and 43 lines here exceed the 80-char limit, so MD013 (and likely MD043/MD060) will produce CI findings. Add the same disable comment as the sibling files to match the established pattern.</violation>
</file>
<file name="src/go/internal/promprofile/semantics/normalizations.go">
<violation number="1" location="src/go/internal/promprofile/semantics/normalizations.go:896">
P2: When exact rules and ranges exhaust the unsigned-integer domain, `categoryOutputPresence` still marks the output optional because `unknown` is configured with `leave_absent`. Determine always-present status from coverage of all valid unsigned values, not only from `unknown.set`, so downstream schema and reduction validation see the required label.</violation>
</file>
<file name="src/go/internal/promprofile/semantics/replay_source.go">
<violation number="1" location="src/go/internal/promprofile/semantics/replay_source.go:378">
P3: When an unsigned_integer-domain label value fails, the new error prints the empty Values slice, so replay failures read as "outside unsigned_integer domain []" rather than explaining the value must be a canonical unsigned integer. Special-case that kind (like labelValueMayMatch does) so the diagnostic is actionable.</violation>
</file>
<file name="src/go/internal/promprofile/semantics/units.go">
<violation number="1" location="src/go/internal/promprofile/semantics/units.go:264">
P3: For a `current` (absolute) `data_rate` component with unit rate `per_second`, `canonicalUnit` returns `bytes/s²`, labelling a plain gauge with a second-derivative unit. The squaring branch is keyed on `rate == "per_second"` rather than on the incremental lifecycle, so it fires for any per_second input even though the only legitimate `bytes/s²` case is a cumulative (incremental) component (which validation forces to rate none). The equivalent `data` quantity appends `/s` once and stays `bytes/s`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,73 @@ | |||
| # SGLang operator model | |||
Contributor
There was a problem hiding this comment.
P2: This new OPERATOR-MODEL.md omits the <!-- markdownlint-disable ... --> header that every sibling OPERATOR-MODEL.md in profile-proofs carries. Codacy runs markdownlint in CI (see .agents/skills/triage-codacy/how-tos/reproduce-pr-22423-markdownlint.md), and 43 lines here exceed the 80-char limit, so MD013 (and likely MD043/MD060) will produce CI findings. Add the same disable comment as the sibling files to match the established pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/go/plugin/go.d/collector/prometheus/profile-proofs/sglang/OPERATOR-MODEL.md, line 1:
<comment>This new OPERATOR-MODEL.md omits the `<!-- markdownlint-disable ... -->` header that every sibling OPERATOR-MODEL.md in profile-proofs carries. Codacy runs markdownlint in CI (see .agents/skills/triage-codacy/how-tos/reproduce-pr-22423-markdownlint.md), and 43 lines here exceed the 80-char limit, so MD013 (and likely MD043/MD060) will produce CI findings. Add the same disable comment as the sibling files to match the established pattern.</comment>
<file context>
@@ -0,0 +1,73 @@
+# SGLang operator model
+
+Start with the endpoint's HTTP activity and completed inference work. Then use model and engine-role views to
</file context>
Suggested change
| # SGLang operator model | |
| <!-- markdownlint-disable MD013 MD043 MD060 --> | |
| # SGLang operator model |
| func categoryOutputPresence(definition Normalization, source SourceLabel) LabelPresence { | ||
| presentAlwaysSets := definition.Malformed.Set != nil && definition.Unknown.Set != nil | ||
| if source.Domain.Kind == "unsigned_integer" { | ||
| presentAlwaysSets = definition.Unknown.Set != nil |
Contributor
There was a problem hiding this comment.
P2: When exact rules and ranges exhaust the unsigned-integer domain, categoryOutputPresence still marks the output optional because unknown is configured with leave_absent. Determine always-present status from coverage of all valid unsigned values, not only from unknown.set, so downstream schema and reduction validation see the required label.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/go/internal/promprofile/semantics/normalizations.go, line 896:
<comment>When exact rules and ranges exhaust the unsigned-integer domain, `categoryOutputPresence` still marks the output optional because `unknown` is configured with `leave_absent`. Determine always-present status from coverage of all valid unsigned values, not only from `unknown.set`, so downstream schema and reduction validation see the required label.</comment>
<file context>
@@ -877,6 +892,9 @@ func categoryOutputSchema(definition Normalization, source SourceLabel) SourceLa
func categoryOutputPresence(definition Normalization, source SourceLabel) LabelPresence {
presentAlwaysSets := definition.Malformed.Set != nil && definition.Unknown.Set != nil
+ if source.Domain.Kind == "unsigned_integer" {
+ presentAlwaysSets = definition.Unknown.Set != nil
+ }
if source.Domain.Kind == "closed" {
</file context>
| if present && schema.Domain.Kind == "closed" && !slices.Contains(schema.Domain.Values, value) { | ||
| return fmt.Errorf("label %q value %q is outside closed domain %v", name, value, schema.Domain.Values) | ||
| if present && schema.Domain.Kind != "open" && !labelValueMayMatch(schema, value) { | ||
| return fmt.Errorf("label %q value %q is outside %s domain %v", name, value, schema.Domain.Kind, schema.Domain.Values) |
Contributor
There was a problem hiding this comment.
P3: When an unsigned_integer-domain label value fails, the new error prints the empty Values slice, so replay failures read as "outside unsigned_integer domain []" rather than explaining the value must be a canonical unsigned integer. Special-case that kind (like labelValueMayMatch does) so the diagnostic is actionable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/go/internal/promprofile/semantics/replay_source.go, line 378:
<comment>When an unsigned_integer-domain label value fails, the new error prints the empty Values slice, so replay failures read as "outside unsigned_integer domain []" rather than explaining the value must be a canonical unsigned integer. Special-case that kind (like labelValueMayMatch does) so the diagnostic is actionable.</comment>
<file context>
@@ -374,8 +374,8 @@ func (c *CompiledSemanticCase) validateProductionSourceLabels(
- if present && schema.Domain.Kind == "closed" && !slices.Contains(schema.Domain.Values, value) {
- return fmt.Errorf("label %q value %q is outside closed domain %v", name, value, schema.Domain.Values)
+ if present && schema.Domain.Kind != "open" && !labelValueMayMatch(schema, value) {
+ return fmt.Errorf("label %q value %q is outside %s domain %v", name, value, schema.Domain.Kind, schema.Domain.Values)
}
delete(observed, name)
</file context>
| unit = "bytes" | ||
| case "data_rate": | ||
| unit = "bytes/s" | ||
| if rate == "per_second" { |
Contributor
There was a problem hiding this comment.
P3: For a current (absolute) data_rate component with unit rate per_second, canonicalUnit returns bytes/s², labelling a plain gauge with a second-derivative unit. The squaring branch is keyed on rate == "per_second" rather than on the incremental lifecycle, so it fires for any per_second input even though the only legitimate bytes/s² case is a cumulative (incremental) component (which validation forces to rate none). The equivalent data quantity appends /s once and stays bytes/s.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/go/internal/promprofile/semantics/units.go, line 264:
<comment>For a `current` (absolute) `data_rate` component with unit rate `per_second`, `canonicalUnit` returns `bytes/s²`, labelling a plain gauge with a second-derivative unit. The squaring branch is keyed on `rate == "per_second"` rather than on the incremental lifecycle, so it fires for any per_second input even though the only legitimate `bytes/s²` case is a cumulative (incremental) component (which validation forces to rate none). The equivalent `data` quantity appends `/s` once and stays `bytes/s`.</comment>
<file context>
@@ -254,6 +259,11 @@ func canonicalUnit(quantity, base, object, rate string) (string, error) {
unit = "bytes"
+ case "data_rate":
+ unit = "bytes/s"
+ if rate == "per_second" {
+ return "bytes/s²", nil
+ }
</file context>
|
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.
Oversized Prometheus metric families currently disappear before profile charts can aggregate them. With a selected profile, apply the existing
max_time_seriesandmax_time_series_per_metriclimits independently to each completed context after aggregation. Omit an oversized context as a whole and reconsider it on subsequent collections, so detailed views can disappear and recover while complete eligible rollups remain available. No new settings or defaults are introduced. Unprofiled collection retains its existing admission behavior.Add a stock SGLang profile covering service/model summaries, workers, scheduling, caches, transfers, multimodal encoding, and HTTP routes. Instances use built-in SGLang identities; deployment-defined request labels aggregate into those identities. Keep worker extrema distinct from additive request totals and preserve histogram components. Include only the semantic proof corrections needed for absent labels, dimension dependencies, numeric status codes, and source units.
Validation:
Companion evidence: netdata/testdata#21. Stock validation needs the paired testdata branch until that dependency is available. Live SGLang validation remains pending because the endpoint is unavailable. Scrape/storage memory still scales with the input population; rendered-series limits do not bound raw input memory.
This remains a draft while the requested LiteLLM and vLLM rollup work continues, in that order.