Duansg commented on PR #4286:
URL: https://github.com/apache/hertzbeat/pull/4286#issuecomment-5242861287
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:
```java
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:
```java
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:
```java
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.
--
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]
