Duansg commented on PR #4286:
URL: https://github.com/apache/hertzbeat/pull/4286#issuecomment-5162770352
The problem is real — labels.putAll(customizedLabels) runs after __name__,
__monitor_id__, __metrics__ and __metric__ are set, so a monitor custom label
using one of those keys silently rewrites the series identity (a custom
__name__ in particular redirects samples into an arbitrary metric name).
Defining an explicit collision policy is the right call, and keeping
job/instance as ordinary labels avoids churning existing series. Two things I'd
like to raise.
addCustomizedLabels re-runs the collision check per sample
saveData() already rejects the batch up front, so by the time the write loop
runs the label map is known to be conflict-free. But addCustomizedLabels now
sits where the old putAll was — inside the per-row × per-numeric-field loop
(VictoriaMetricsDataStorage.java:217-244) — and each call re-executes:
Set<String> collisions = new TreeSet<>(customizedLabels.keySet());
collisions.retainAll(MANAGED_LABEL_KEYS);
That's a fresh TreeSet built from every custom label key, plus a retainAll,
for every sample. A monitor emitting 1,000 rows × 20 numeric fields does 20,000
redundant allocations per batch, on the hottest path in the write pipeline. The
IllegalArgumentException inside it is unreachable for the same reason.
The cluster variant is worse: there the for (Map.Entry<String, Double> ...)
loop is nested inside the cellStream().forEach(cell -> {...}) lambda
(VictoriaMetricsClusterDataStorage.java:218-262), so the call count is rows ×
cells × fields. That nesting looks like a pre-existing bug — it also appears to
emit duplicate VictoriaMetricsContent entries — and it's out of scope here, but
it does multiply the cost of this change.
Suggestion: keep the single entry-point validation and let the inner loop do
a plain labels.putAll(customizedLabels).
Rejecting the whole batch is itself silent data loss
The PR description says the goal is to "not silently discard or rename user
labels", but dropping the entire metrics batch discards strictly more than
renaming one label would. From an operator's point of view a single mistyped
label key makes all metrics for that monitor disappear, with only an ERROR line
in the log to explain it.
Fail-closed is defensible, but it would help to make the failure observable
beyond a log line — a counter, or surfacing it on the monitor itself — since
the affected monitor otherwise just looks like it stopped reporting.
Alternatively, dropping only the conflicting keys and keeping the sample
preserves the data while still protecting series identity.
Coverage
GreptimeDbDataStorage.java:96 declares the same LABEL_KEY_NAME = "__name__"
and follows the same "set managed labels, then putAll custom labels" pattern,
so it has the identical collision. Worth either covering it here or noting it
as a follow-up so it doesn't get lost.
Minor
OutputCaptureExtension assertions on log text are sensitive to logging
configuration; asserting on a returned/collected value would be more robust if
there's a natural seam for it. Not blocking.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
