maintenance: define VictoriaMetrics label collisions

[zqr10159]

Summary

• define an explicit collision policy shared by the single-node and cluster VictoriaMetrics writers
• keep job and instance as ordinary Prometheus custom labels, preserving the exact label set and series identity used before this PR
• reject a metrics batch when monitor custom labels use HertzBeat-managed keys: __name__, __monitor_id__, __metrics__, or __metric__
• report conflicting key names without logging their values; do not silently discard or rename user labels
• document the upgrade check and clarify that existing VictoriaMetrics series are not rewritten

Upgrade behavior

• monitors using job, instance, or ordinary custom labels require no migration and continue writing the same label set
• before upgrade, rename any monitor custom label using one of the four HertzBeat-managed keys
• a remaining managed-key collision is observable and fail-closed: that metrics batch is not sent, and the application log lists only the conflicting key names

Validation

• regression contracts first failed on the previous head: the writer changed existing job/instance values and silently accepted managed-key collisions
• ./mvnw -pl hertzbeat-warehouse -Dtest=VictoriaMetricsDataStorageTest test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false
  • 6 tests passed; Checkstyle passed
• ./mvnw -pl hertzbeat-warehouse -am test -DskipITs
  • complete reactor passed; hertzbeat-warehouse ran 61 tests with 0 failures and 0 errors
• git diff --cached --check

AI assistance: used for draft implementation and test iteration.
Human validation: the serialized write payload retains existing custom job and instance values, managed-key collisions produce a value-free diagnostic and no HTTP write, and the complete warehouse reactor passed locally.
Risk notes: monitors that currently use a HertzBeat-managed custom-label key stop writing VictoriaMetrics batches until the key is renamed. This is intentional and documented so the upgrade cannot silently split or relabel an existing series.

[zqr10159]

Author remediation update:

The collision policy is now explicit and shared by both VictoriaMetrics writers. Existing Prometheus-style job, instance, and ordinary custom labels are preserved exactly. HertzBeat-managed keys are rejected with a key-only diagnostic and no write; user labels are not silently discarded or renamed. The upgrade note identifies the four managed keys that must be renamed.

Payload-continuity and no-write diagnostic tests passed (6 focused tests), and the full warehouse reactor passed (61 tests). Backend, E2E, license, and label checks are green. DOC CI failed only because an unchanged historical Gitee link returned HTTP 405; that failed job has been rerun. Maintainer review remains required.

[zqr10159]

CI follow-up: the DOC CI rerun passed, including Dead Link Check and the documentation build. Backend, Maven E2E, image E2E, license, and label checks are also green; all current checks have completed successfully.

[Duansg]

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 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.

[zqr10159]

Addressed the review findings in commit b87ff44df7.

Changes:

• VictoriaMetrics collision validation now runs once per incoming batch instead of allocating/revalidating for every emitted sample;
• single-node and cluster VictoriaMetrics storage count rejected collision batches and include the cumulative count in value-free diagnostics;
• Greptime now uses the same explicit reject policy for custom labels that collide with instance, ts, or metric fields instead of silently skipping them;
• non-conflicting Greptime custom tags remain supported.

Human validation:

• full VictoriaMetricsDataStorageTest,GreptimeDbDataStorageTest suites passed;
• focused collision and allowed-label contracts passed;
• git diff --check.

The new-head backend, Maven E2E, license, docs, and label checks are currently queued by GitHub and have not started yet.

AI assistance: used for draft implementation and test iteration.
Risk notes: collision rejection is intentionally observable and batch-scoped; it does not rewrite or namespace user labels.

@Duansg, please re-review this head when convenient.

[Duansg]

Rejecting the whole batch is a regression for GreptimeDB, and the VictoriaMetrics guard misses the one key users actually collide with

GreptimeDB already handles this, non-destructively. GreptimeDbDataStorage.saveData skips the offending key and stores everything else:

for (Map.Entry<String, String> label : customLabels.entrySet()) {
    String key = label.getKey();
    if (!LABEL_KEY_INSTANCE.equals(key) && !LABEL_KEY_TS.equals(key) && !fieldNames.contains(key)) {
        tableSchemaBuilder.addTag(key, DataType.String);
        labelKeys.add(key);
    }
}

This PR inserts a pre-check whose condition is the exact inverse of that one, and returns before reaching it:

Set<String> labelCollisions = findLabelCollisions(customLabels, fieldNames);
if (!labelCollisions.isEmpty()) {
    long rejectedCount = rejectedLabelCollisionCount.incrementAndGet();
    log.error("[warehouse greptime] reject metrics data {} ...", ...);
    return;
}

Two consequences. The behaviour goes from "drop one label, keep the metrics" to "drop every metric in the batch", and the loop above becomes unreachable for exactly the case it was written for.

The trigger is not exotic: findLabelCollisions includes fieldNames.contains(key), and monitor labels are free-form user-entered keys. A label named status, total, usage, cores — anything matching a field name in that metric set — now silently stops all storage for that monitor, with one log.error as the only signal. Nothing surfaces in the UI.

VictoriaMetrics is the storage that actually needs a guard, and it doesn't get one where it matters. Master applies user labels last and unconditionally:

labels.put(LABEL_KEY_MONITOR_ID, String.valueOf(metricsData.getId()));
var customizedLabels = metricsData.getLabels();
if (!ObjectUtils.isEmpty(customizedLabels)) {
    labels.putAll(customizedLabels);   // overwrites anything already in `labels`
}

so a user label does override a managed one — a real bug. But the new MANAGED_LABEL_KEYS covers only name, monitor_id, metrics, metric. Those are double-underscore internal names no operator is going to type. Meanwhile LABEL_KEY_INSTANCE is the literal string instance (VictoriaMetricsDataStorage.java:96), it is placed into defaultLabels before the putAll, and it is not in the managed set — so the one collision that will plausibly happen still silently rewrites the instance label. GreptimeDB has always guarded instance; VM still doesn't.

Net effect across the two storages: the one that was safe now loses data, and the one that was unsafe stays unsafe for the realistic case.

Suggested direction

1. Keep skip semantics rather than batch rejection. GreptimeDB's existing loop is the right model — drop the colliding key, store the rest, and keep the counter and a rate-limited warn so operators can see it.
2. Apply the same skip in addCustomizedLabels on the VM side instead of a bare putAll, and add LABEL_KEY_INSTANCE to MANAGED_LABEL_KEYS. Aligning the two managed-key sets would be worth doing in this PR, since the whole point is consistent collision handling.
3. The durable fix is upstream of ingestion: reject a reserved label key when the monitor's labels are saved in the manager, where the user can actually see and correct the error. Storage-side handling should be a non-destructive backstop, not the enforcement point.

On the rest of the PR: hoisting the collision computation out of the row × field inner loop to the saveData entry point is the right placement, and covering GreptimeDbDataStorage alongside both VM storages is good coverage. The disagreement is only about what to do on a hit — return is too blunt for something an operator can trigger by naming a label.