[coordinator] Support mapping write tags via a header #2255
Conversation
Implements and closes #2254.
| return 0 | ||
| } | ||
|
|
||
| function prometheus_query_native { |
There was a problem hiding this comment.
Would it be worth pulling these tests into an existing prom remote write test? Then can avoid overhead in setting up the stack
There was a problem hiding this comment.
Agreed, it is starting to get copy/pasted into a lot of different places. Unfortunately bash does tend to trend towards this pattern.. at least the way we're writing it/etc since we're not paying a lot of attention/have a lot of experience with DRY bash.
There was a problem hiding this comment.
My take though too: would be great if we could actually move this into common.sh.
| } | ||
|
|
||
| // No existing labels with this tag, append it. | ||
| req.Timeseries[i].Labels = append(ts.Labels, prompb.Label{ |
There was a problem hiding this comment.
This may need to insert in sorted order
There was a problem hiding this comment.
Hm @arnikola do we normalize later?
There was a problem hiding this comment.
If we move this to PromLabelsToM3Tags it calls Normalize() at the end
EDIT: Realized this is called after ParseRequest anyway. So we can either allow tags to temporarily be unsorted and wait until they're normalized, or insert in sorted order.
| tag := []byte(op.Tag) | ||
| value := []byte(op.Value) | ||
|
|
||
| TSLoop: |
There was a problem hiding this comment.
nit: not sure if we use many labeled continues elsewhere in the codebase, not sure if we want to keep the style choice here?
There was a problem hiding this comment.
Yeah I'd probably opt for a bool then checking the bool after the loop with a break, but yeah - is a nit
| return fmt.Errorf("must specify one operation per tag mapper (got %d)", numOps) | ||
| } | ||
|
|
||
| // AppendOp with value tag="foo" and value="bar" will unconditionally add |
There was a problem hiding this comment.
nit: maybe change the name from append to write or something? Drop and Replace seem to describe their operations well enough, but this is more of an addOrUpdate than an append?
|
|
||
| // DropOp with tag="foo" and an empty value will remove all tag-value pairs in | ||
| // all timeseries in the write request where the tag was "foo". If value is | ||
| // non-empty, a tag-value pair will only be removed if the value was equal to |
There was a problem hiding this comment.
nit: maybe separate op for drop tag vs drop tag with value in the future?
Codecov Report
@@ Coverage Diff @@
## master #2255 +/- ##
========================================
- Coverage 71.3% 66.7% -4.7%
========================================
Files 1023 1022 -1
Lines 89100 89090 -10
========================================
- Hits 63574 59456 -4118
- Misses 21160 25240 +4080
- Partials 4366 4394 +28
Continue to review full report at Codecov.
|
robskillington
changed the title
| @@ -60,6 +60,10 @@ const ( | |||
| // in JSON format. See `handler.stringTagOptions` for definitions.` | |||
| RestrictByTagsJSONHeader = M3HeaderPrefix + "Restrict-By-Tags-JSON" | |||
|
|
|||
| // MapTagsByJSONHeader provides the ability to mutate tags of timeseries in | |||
| // incoming write requests. See `MapTagsOptions` for structure. | |||
| MapTagsByJSONHeader = "M3-Map-Tags-By-JSON" | |||
There was a problem hiding this comment.
Can you make this M3HeaderPrefix + "Map-Tags-JSON"? Can we remove the word "By" here? For the restrict it's the term "Restrict-By" then "Tags" then "JSON" this is "Map" then "Tags" then the "By" is before JSON (which is strange).
I think it should probably be "Map-By-Tags-JSON" or just "Map-Tags-JSON".
Implements and closes #2254.
What this PR does / why we need it:
Fixes # 2254
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: