remote-write: Add type and unit labels to receiver when feature flag enabled#17329
remote-write: Add type and unit labels to receiver when feature flag enabled#17329bwplotka merged 7 commits intoprometheus:mainfrom
Conversation
Signed-off-by: Harsh <[email protected]>
hxrshxz
force-pushed
the
type-and-unit-labels
branch
from
78364cf to
0f98c24
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where type and unit labels were not being propagated to the Prometheus receiver when using Remote Write v2 with the type-and-unit-labels feature flag enabled. The fix adds the feature flag parameter to the write handler constructor and implements logic to add __type__ and __unit__ labels from metadata when the flag is enabled.
- Passes the
enableTypeAndUnitLabelsparameter to the remote write handler constructor - Adds logic to extract type and unit metadata and convert them to labels in Remote Write v2 processing
- Includes comprehensive test coverage for various scenarios with and without the feature flag
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/api/v1/api.go | Passes the enableTypeAndUnitLabels parameter to NewWriteHandler |
| storage/remote/write_handler.go | Adds feature flag field and implements type/unit label addition logic |
| storage/remote/write_handler_test.go | Updates all test calls to include the new parameter |
| storage/remote/write_handler_type_unit_test.go | Adds comprehensive test coverage for the new functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Harsh <[email protected]>
hxrshxz
force-pushed
the
type-and-unit-labels
branch
from
54c2a0b to
11035f6
Compare
Signed-off-by: Harsh <[email protected]>
There was a problem hiding this comment.
Thanks!
Left some comments, let me know if those make sense! Generally looks good, just a few pointers around code sustainability and reliability of tests.
Also, do you mind changing [BUGFIX] in PR description to [FEATURE]? This is not a bug just an iterative implementation.
Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: harsh kumar <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: harsh kumar <[email protected]>
…r tests Signed-off-by: Harsh <[email protected]>
hxrshxz
force-pushed
the
type-and-unit-labels
branch
from
62433a1 to
cb47b6b
Compare
Signed-off-by: Harsh <[email protected]>
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Looks great! LGTM, just another suggestion to make things much more efficient.
|
@bwplotka I agree this adds some overhead Maybe we can merge this with the TODO for now and optimize ts.ToLabels() in a follow up. If that sounds good then I can raise a new issue for this and make the changes there? |
|
👍🏽 |
Which issue(s) does the PR fix:
Fixes #17319
Does this PR introduce a user-facing change?