chenBright commented on PR #3557:
URL: https://github.com/apache/brpc/pull/3557#issuecomment-5770409472
> Is this change incompatible with some existing user code? Consider add a
flag
@wwbmmm These conflicts are not introduced by this PR. They were already
reachable,
Histogram only makes them easy to hit: one Histogram maps to three sample
names
(`_bucket`, `_sum`, `_count`) and reserves the `le` label, so names and
labels
that used to look free now collide. The PR fixes the underlying problems
instead
of working around them for Histogram.
What can now be rejected and was not before:
1. A bvar and an mbvar exposing the same Prometheus name. The two used to
keep
separate namespaces, so `/brpc_metrics` could print two different metrics
under one name. They now share one registry and the second expose fails.
Nothing changes among plain bvars, because the Prometheus name of a bvar
is
its own name, which `VarMap` already keys on.
2. Exposing an mbvar under a name that is already taken.
`MVariableBase::expose_impl` used to return 0 without exposing anything.
It
now returns -1, like `Variable::expose`.
3. The names a `LatencyRecorder` synthesizes for Prometheus (`X`, `X_sum`,
`X_avg_latency`) are reserved in `MethodStatus::Expose`, so a user bvar
can
no longer take one of them. `Server::Start` ignores this return value, so
no
server fails to start because of it.
4. `dump()` now writes a value only if it is a valid Prometheus number. It
used
to skip only values starting with `"`, so a bool gflag (`true`), the json
of
a compound `PassiveStatus` (`{...}`) and `-nan` all went out as metric
values.
5. A MultiDimension whose label name collides with one its value type
reserves.
It logs an error and refuses to expose. Recording is untouched, so
`*md.get_stats({...}) << v` still works and still returns a valid pointer.
None of this rejects anything that used to work. In every case the old
behaviour
produced output Prometheus cannot parse, and one bad line fails the entire
scrape, not just that metric. So the real choice is between a metric that
silently never arrives and an error log at expose time.
That is also why I do not think a flag is worth it. To help anyone it would
have
to default to the old behaviour, which keeps the broken scrape broken by
default, and what it preserves is not a working metric.
`-bvar_abort_on_same_name` already exists for users who want name conflicts
to
be fatal rather than logged.
--
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]
