Copilot commented on code in PR #13865:
URL: https://github.com/apache/skywalking/pull/13865#discussion_r3212783936
##########
docs/en/setup/backend/admin-api/dsl-debugging-mal.md:
##########
@@ -181,7 +181,17 @@ A trimmed slice (one record = one scrape window):
{ "type": "output",
"sourceText": "e2e_demo_filtered_requests",
"continueOn": true,
- "payload": { /* terminal meter sample — metric, entity, value,
timeBucket */ } }
+ "payload": {
+ "metric": "e2e_demo_filtered_requests",
+ "entity": "MeterEntity(scopeType=SERVICE, serviceName=my-svc, …)",
+ "valueType": "sum",
+ "timeBucket": 202605091036,
+ "value": 42 /* shape depends on valueType:
+ number for
Sum/Avg/Max/Min/CPM/Latest…,
+ object {bucket: count} for
histograms /
+ *Labeled functions, omitted for
non-scalar
+ holders. NaN/±Infinity render as
strings. */
+ } }
Review Comment:
This doc update adds `payload.value` to the example, but the later
**Limits** section still says `recordCap` default is `1000` and even suggests
`{ "recordCap": 200 }`. With `SessionLimits.MAX_RECORD_CAP` now 100, the
default and override example should be updated to <=100 to match actual API
validation (otherwise users following the docs will get `400 invalid_limits`).
##########
oap-server/server-admin/dsl-debugging/src/main/java/org/apache/skywalking/oap/server/admin/dsl/debugging/mal/MALDebugRecorderImpl.java:
##########
@@ -185,9 +191,100 @@ private static String meterEmitPayload(final MeterEntity
entity, final String me
obj.addProperty("entity", entity == null ? null : entity.toString());
obj.addProperty("valueType", resolveFunctionName(value));
obj.addProperty("timeBucket", timeBucket);
+ appendValue(obj, value);
return obj.toString();
}
+ /**
+ * Surface the metric's terminal reading on the captured {@code output}
+ * sample so the operator sees the actual MAL emission, not just the
+ * function name. The shape of {@code "value"} depends on the holder
+ * the generated function class implements:
+ * <ul>
+ * <li>{@link LongValueHolder} / {@link IntValueHolder} → JSON number
+ * (Sum, Avg, Max, Min, Latest, SumPerMin, …).</li>
+ * <li>{@link DoubleValueHolder} → JSON number; non-finite values
+ * (NaN, ±Infinity) render as the corresponding string so the
+ * wire stays valid JSON and the reading is still visible.</li>
+ * <li>{@link LabeledValueHolder} → JSON object {@code {key: long}}
+ * — labeled metrics ({@code *Labeled}) and histogram/percentile
+ * functions ({@code AvgHistogramPercentileFunction},
+ * {@code SumHistogramPercentileFunction}) whose reading is a
+ * {@link DataTable}. Keys are label combos for {@code *Labeled},
+ * {@code p=<rank>} entries for percentile functions.</li>
+ * </ul>
+ * If {@code value} is null or not one of the recognised holders the
+ * field is omitted; the operator still sees {@code valueType} and can
+ * tell from the function name that the shape is non-scalar.
+ *
+ * <p><b>Two-phase functions.</b> Some functions split work between
+ * {@code accept()} and {@code calculate()} — accept() populates raw
+ * aggregates (e.g. {@code summation} + {@code count} for histogram-
+ * percentile), and calculate() turns those into the user-visible
+ * field returned by {@code getValue()} (e.g. {@code percentileValues}).
+ * The MAL {@code captureMeterEmit} probe fires AFTER accept() but
+ * BEFORE the streaming pipeline calls calculate(), so without forcing
+ * calculate() here the labeled value column would be an empty map for
+ * histogram-percentile rules — exactly when operators most need to
+ * verify what the rule emits. We force calculate() at probe time so
+ * the captured value matches what the storage row will contain. This
+ * is safe and zero-cost when debug is off:
+ * <ul>
+ * <li>The probe site itself is gated; when no operator has installed
+ * a session the call into this method is dead code that the JIT
+ * elides on hot-path inlining.</li>
+ * <li>{@code calculate()} is idempotent via the function's
+ * {@code isCalculated} guard, so the streaming pipeline's later
+ * call is a no-op rather than double work.</li>
+ * <li>Cross-node {@code combine()} resets {@code isCalculated=false},
+ * so the post-merge calculate on combined state still happens
+ * on read — pre-computing here on the local snapshot doesn't
+ * leak stale values into the cluster.</li>
Review Comment:
The new javadoc claims the streaming pipeline’s later `calculate()` call
becomes a no-op due to an `isCalculated` guard, but many `Metrics.calculate()`
implementations aren’t idempotent/no-op (e.g., `AvgFunction.calculate()` always
recomputes, and
`AvgHistogramPercentileFunction`/`SumHistogramPercentileFunction` don’t set
`isCalculated=true` at all). This makes the explanation misleading; please
reword to avoid asserting no extra work/true idempotence (or narrow the claim
to specific implementations you’ve verified).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
