Add type and unit labels to prw2 #7077
Conversation
SungJin1212
force-pushed
the
Add-type-and-unit-labels-to-prw2
branch
from
c14d273 to
5ab3a6a
Compare
| # EXPERIMENTAL: If true, the __type__ and __unit__ labels are added to metrics. | ||
| # This only applies to remote write v2 requests. | ||
| # CLI flag: -distributor.rw2-enable-type-and-unit-labels |
There was a problem hiding this comment.
Do we need to mention rw2 in the config name? I would align the config name with Prometheus and callout that this feature is only for RW2 in the description.
There was a problem hiding this comment.
Then this flag has same name with the -distributor.otlp.enable-type-and-unit-labels field. Doesn't it matter?
There was a problem hiding this comment.
My concern is that users don't know what rw2 means. We should avoid using abbreviations in the config name. I think it should be ok to use 1 flag to control both. Maybe we can unify to be distributor.enable-type-and-unit-labels?
There was a problem hiding this comment.
I add -distributor.enable-type-and-unit-labels flag which works for OTLP and PRW2. I think this PR should be included in version 1.20.
SungJin1212
force-pushed
the
Add-type-and-unit-labels-to-prw2
branch
4 times, most recently
from
52703db to
b19b5cf
Compare
SungJin1212
force-pushed
the
Add-type-and-unit-labels-to-prw2
branch
2 times, most recently
from
28e61e7 to
c860a65
Compare
|
I think one issue today for this feature is that type and unit labels are not correctly handled by thanos engine. They are handled only by promql engine. Maybe we need to call it out in the doc or the config description |
|
@yeya24 |
… and otlp Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Add-type-and-unit-labels-to-prw2
branch
from
59fc644 to
6dfdc80
Compare
|
@yeya24 |
yeya24
left a comment
yeya24
left a comment
There was a problem hiding this comment.
I think we should separate out updating thanos engine to a separate PR instead of doing it here to make the PR size smaller. We don't have to do them in the same PR.
But if you already did it then it is ok
CHANGELOG.md
Outdated
| ## master / unreleased | ||
|
|
||
| * [FEATURE] StoreGateway: Introduces a new parquet mode. #7046 | ||
| * [FEATURE] Distributor: Add a per-tenant flag `-distributor.enable-type-and-unit-labels` which enables to add `__unit__` and `__type__` labels for remote write v2 and OTLP requests. The `-distributor.otlp.enable-type-and-unit-labels` flag has been consolidated into this flag. #7077 |
There was a problem hiding this comment.
Is it a breaking change? I think this is fine as that seems an experimental feature. But we do need to mention that it is a breaking change.
There was a problem hiding this comment.
Thanks for the suggestion, since the changes are already integrated and tested together, I'd prefer to keep them in this PR. Also, I added breaking change mention to change log.
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
2 participants
Add a per-tenant flag
-distributor.enable-type-and-unit-labelswhich enables to add__unit__and__type__labels for remote write v2 and OTLP requests.Which issue(s) this PR fixes:
Fixes #7059
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]