You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "hanahmily (via GitHub)" <gi...@apache.org> on 2026/06/04 00:51:44 UTC

[PR] Thread Prometheus metric type end-to-end through FODC agent and proxy [skywalking-banyandb]

hanahmily opened a new pull request, #1153:
URL: https://github.com/apache/skywalking-banyandb/pull/1153

   ## Problem
   
   `fodc-proxy` silently corrupts Prometheus metric **types**. On a live cluster the proxy `/metrics` exposed **0 counters and 0 summaries** — every counter was emitted as `gauge`, several `_count`-suffixed counters as `histogram`, and `go_gc_duration_seconds` as two conflicting `# TYPE` lines.
   
   ## Root cause
   
   Type information was destroyed before the proxy could ever emit it, across the whole pipeline `banyandb /metrics → agent → gRPC StreamMetrics → proxy /metrics`:
   
   1. **Agent parser** captured `# HELP` but dropped `# TYPE` entirely (`RawMetric` had no type field).
   2. **Flight recorder** stored descriptions but no types.
   3. **`StreamMetrics` proto** `Metric` had no type field.
   4. **Proxy aggregator** `AggregatedMetric` had no type field.
   5. **Proxy formatter** guessed the type from a name-suffix heuristic: anything ending `_bucket/_sum/_count` → `histogram`, else `gauge`. This downgraded counters, mislabeled banyandb's `_count`-suffixed counters as histograms (breaking `histogram_quantile`), and split summaries into conflicting `histogram`+`gauge` lines (which Prometheus rejects).
   
   ## Fix
   
   Thread the real metric type end-to-end:
   
   - **Capture** the type using the Prometheus `expfmt` parser (authoritative `MetricFamily.Type`), resolving histogram/summary component series (`_bucket/_sum/_count`) to their family type by base name. The existing line-based value/label extraction is unchanged, so wire fidelity is preserved.
   - **Store** it alongside descriptions in the flight recorder (`Datasource.types` + `GetTypes()`).
   - **Forward** it via a new additive `Metric.type` enum (`MetricType`) over gRPC.
   - **Emit** the real type from the proxy. The proxy groups by family base and emits exactly one `# TYPE` line per family.
   - **Mixed-version safety:** pre-upgrade agents send `METRIC_TYPE_UNSPECIFIED`; the proxy folds those untyped samples into the matching typed family (and falls back to the legacy heuristic for fully-untyped families), so a rolling upgrade never produces two conflicting `# TYPE` lines for one metric.
   - The agent's own `/metrics` exporter now maps counter/gauge correctly and degrades histogram/summary scalars to `untyped` (`NewConstMetric` can't reconstruct them).
   
   ## Tests
   
   - Parser test: counter/gauge/histogram type propagation incl. base-name resolution for component series.
   - Proxy formatter tests: counter stays `counter`; a `_count`-suffixed counter is **not** folded into a histogram; histogram and summary families each emit exactly one `# TYPE` line; and two mixed-version dedup tests (typed + untyped same family → single authoritative `# TYPE` line, both samples retained).
   
   ## Verification on a live cluster
   
   Built the images, rolled the agent sidecar (7 banyandb pods) + proxy on a live showcase cluster, and scraped the proxy `/metrics` before/after:
   
   | `# TYPE` distribution | counter | gauge | histogram | summary |
   |---|---|---|---|---|
   | Before | 0 | 367 | 35 | 0 |
   | After  | 113 | 255 | 7 | 1 |
   
   `rate()`/`increase()` on counters, `histogram_quantile()` on real histograms, and the GC summary quantiles all work again; no metric name has more than one `# TYPE` line.
   
   Note: `api/.../rpc.pb.go` is generated (gitignored) and regenerated by CI from the `rpc.proto` change in this PR.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Thread Prometheus metric type end-to-end through FODC agent and proxy [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1153:
URL: https://github.com/apache/skywalking-banyandb/pull/1153


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Thread Prometheus metric type end-to-end through FODC agent and proxy [skywalking-banyandb]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1153:
URL: https://github.com/apache/skywalking-banyandb/pull/1153#discussion_r3353409941


##########
fodc/proxy/internal/api/server.go:
##########
@@ -354,31 +409,71 @@ func (s *Server) formatPrometheusText(aggregatedMetrics []*metrics.AggregatedMet
 		regularNames = append(regularNames, name)
 	}
 	sort.Strings(regularNames)
-
 	for _, name := range regularNames {
-		group := regularMetrics[name]
-		if group.description != "" {
-			builder.WriteString(fmt.Sprintf("# HELP %s %s\n", group.name, group.description))
+		grp := regularMetrics[name]
+		if grp.description != "" {
+			builder.WriteString(fmt.Sprintf("# HELP %s %s\n", grp.name, grp.description))
+		}
+		builder.WriteString(fmt.Sprintf("# TYPE %s gauge\n", grp.name))
+		for _, m := range grp.metrics {
+			builder.WriteString(formatMetricLine(m))
 		}
-		builder.WriteString(fmt.Sprintf("# TYPE %s gauge\n", group.name))
+	}
+}
 
-		for _, metric := range group.metrics {
-			labelParts := make([]string, 0, len(metric.Labels))
-			for key, value := range metric.Labels {
-				labelParts = append(labelParts, fmt.Sprintf(`%s="%s"`, key, value))
+// matchTypedFamily returns the typed family an untyped series belongs to, or nil if none.
+// It checks the exact name first (counter/gauge/untyped series and bare summary quantile
+// series), then the histogram/summary component base after trimming a trailing
+// _bucket/_sum/_count suffix. Used to fold pre-upgrade (untyped) samples into the
+// authoritative typed family so the proxy never emits two TYPE lines for one name.
+func matchTypedFamily(typedFamilies map[string]*metricGroup, name string) *metricGroup {
+	if grp, ok := typedFamilies[name]; ok {
+		return grp
+	}
+	for _, suffix := range []string{"_bucket", "_sum", "_count"} {
+		if strings.HasSuffix(name, suffix) {
+			base := strings.TrimSuffix(name, suffix)
+			if base != "" {
+				if grp, ok := typedFamilies[base]; ok {
+					return grp
+				}
 			}
-			sort.Strings(labelParts)
+			break
+		}
+	}
+	return nil
+}
 
-			labelStr := ""
-			if len(labelParts) > 0 {
-				labelStr = "{" + strings.Join(labelParts, ",") + "}"
+// typedFamilyBase returns the Prometheus family base name for a typed series.
+// For histogram/summary component series the trailing _bucket/_sum/_count is stripped.
+// For all other types (counter, gauge, untyped) the name is used as-is.
+func typedFamilyBase(name, metricType string) string {
+	switch metricType {
+	case "histogram", "summary":
+		for _, suffix := range []string{"_bucket", "_sum", "_count"} {
+			if strings.HasSuffix(name, suffix) {
+				base := strings.TrimSuffix(name, suffix)
+				if base != "" {
+					return base
+				}
 			}
-
-			builder.WriteString(fmt.Sprintf("%s%s %s\n", group.name, labelStr, formatFloat(metric.Value)))
 		}
 	}
+	return name
+}
 
-	return builder.String()
+// formatMetricLine renders a single metric sample as a Prometheus text line.
+func formatMetricLine(m *metrics.AggregatedMetric) string {
+	labelParts := make([]string, 0, len(m.Labels))
+	for k, v := range m.Labels {
+		labelParts = append(labelParts, fmt.Sprintf(`%s="%s"`, k, v))
+	}
+	sort.Strings(labelParts)
+	labelStr := ""
+	if len(labelParts) > 0 {
+		labelStr = "{" + strings.Join(labelParts, ",") + "}"
+	}
+	return fmt.Sprintf("%s%s %s\n", m.Name, labelStr, formatFloat(m.Value))
 }

Review Comment:
   Prometheus text exposition requires label values to be escaped (\, ", and newlines). formatMetricLine currently interpolates raw label values into `key="value"`, which can produce invalid `/metrics` output if any label contains special characters, causing Prometheus scrape failures.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Thread Prometheus metric type end-to-end through FODC agent and proxy [skywalking-banyandb]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1153:
URL: https://github.com/apache/skywalking-banyandb/pull/1153#issuecomment-4618390646

   ## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1153?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 41.17%. Comparing base ([`3530dd9`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/3530dd9b5cefaa4fef60eec91ecbfbdfcaf243c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f62181b`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/f62181b1d42a92f472a9c4d097296260f35b5b4c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 274 commits behind head on main.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1153      +/-   ##
   ==========================================
   - Coverage   45.97%   41.17%   -4.81%     
   ==========================================
     Files         328      196     -132     
     Lines       55505    20380   -35125     
   ==========================================
   - Hits        25520     8391   -17129     
   + Misses      27909    11139   -16770     
   + Partials     2076      850    -1226     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1153/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [pkg](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1153/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.17% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1153?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Thread Prometheus metric type end-to-end through FODC agent and proxy [skywalking-banyandb]

Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #1153:
URL: https://github.com/apache/skywalking-banyandb/pull/1153#discussion_r3353428273


##########
fodc/proxy/internal/api/server.go:
##########
@@ -354,31 +409,71 @@ func (s *Server) formatPrometheusText(aggregatedMetrics []*metrics.AggregatedMet
 		regularNames = append(regularNames, name)
 	}
 	sort.Strings(regularNames)
-
 	for _, name := range regularNames {
-		group := regularMetrics[name]
-		if group.description != "" {
-			builder.WriteString(fmt.Sprintf("# HELP %s %s\n", group.name, group.description))
+		grp := regularMetrics[name]
+		if grp.description != "" {
+			builder.WriteString(fmt.Sprintf("# HELP %s %s\n", grp.name, grp.description))
+		}
+		builder.WriteString(fmt.Sprintf("# TYPE %s gauge\n", grp.name))
+		for _, m := range grp.metrics {
+			builder.WriteString(formatMetricLine(m))
 		}
-		builder.WriteString(fmt.Sprintf("# TYPE %s gauge\n", group.name))
+	}
+}
 
-		for _, metric := range group.metrics {
-			labelParts := make([]string, 0, len(metric.Labels))
-			for key, value := range metric.Labels {
-				labelParts = append(labelParts, fmt.Sprintf(`%s="%s"`, key, value))
+// matchTypedFamily returns the typed family an untyped series belongs to, or nil if none.
+// It checks the exact name first (counter/gauge/untyped series and bare summary quantile
+// series), then the histogram/summary component base after trimming a trailing
+// _bucket/_sum/_count suffix. Used to fold pre-upgrade (untyped) samples into the
+// authoritative typed family so the proxy never emits two TYPE lines for one name.
+func matchTypedFamily(typedFamilies map[string]*metricGroup, name string) *metricGroup {
+	if grp, ok := typedFamilies[name]; ok {
+		return grp
+	}
+	for _, suffix := range []string{"_bucket", "_sum", "_count"} {
+		if strings.HasSuffix(name, suffix) {
+			base := strings.TrimSuffix(name, suffix)
+			if base != "" {
+				if grp, ok := typedFamilies[base]; ok {
+					return grp
+				}
 			}
-			sort.Strings(labelParts)
+			break
+		}
+	}
+	return nil
+}
 
-			labelStr := ""
-			if len(labelParts) > 0 {
-				labelStr = "{" + strings.Join(labelParts, ",") + "}"
+// typedFamilyBase returns the Prometheus family base name for a typed series.
+// For histogram/summary component series the trailing _bucket/_sum/_count is stripped.
+// For all other types (counter, gauge, untyped) the name is used as-is.
+func typedFamilyBase(name, metricType string) string {
+	switch metricType {
+	case "histogram", "summary":
+		for _, suffix := range []string{"_bucket", "_sum", "_count"} {
+			if strings.HasSuffix(name, suffix) {
+				base := strings.TrimSuffix(name, suffix)
+				if base != "" {
+					return base
+				}
 			}
-
-			builder.WriteString(fmt.Sprintf("%s%s %s\n", group.name, labelStr, formatFloat(metric.Value)))
 		}
 	}
+	return name
+}
 
-	return builder.String()
+// formatMetricLine renders a single metric sample as a Prometheus text line.
+func formatMetricLine(m *metrics.AggregatedMetric) string {
+	labelParts := make([]string, 0, len(m.Labels))
+	for k, v := range m.Labels {
+		labelParts = append(labelParts, fmt.Sprintf(`%s="%s"`, k, v))
+	}
+	sort.Strings(labelParts)
+	labelStr := ""
+	if len(labelParts) > 0 {
+		labelStr = "{" + strings.Join(labelParts, ",") + "}"
+	}
+	return fmt.Sprintf("%s%s %s\n", m.Name, labelStr, formatFloat(m.Value))
 }

Review Comment:
   Good catch — fixed in 0773ab81. `formatMetricLine` now escapes label values via a `strings.NewReplacer` (`\` → `\\`, `"` → `\"`, newline → `\n`) in a single pass, and `TestFormatPrometheusText_EscapesLabelValues` covers a value containing all three special characters.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org