[coordinator] Treat Prometheus TagName/Value as []byte instead of String #1004
Conversation
| @@ -91,5 +92,6 @@ func TestAndWithSomeValues(t *testing.T) { | |||
| expected := values1 | |||
| expected[0][1] = math.NaN() | |||
| expected[1][0] = math.NaN() | |||
| fmt.Println(expected, sink.Values) | |||
There was a problem hiding this comment.
Oops yeah made a mental note to clean these but forgot it unfortunately
prateek
changed the title
| @@ -120,11 +120,11 @@ func (w *downsamplerFlushHandlerWriter) Write( | |||
| tags := make(models.Tags, 0, expected+1) | |||
There was a problem hiding this comment.
nit: This should actually just be expected not expected+1 since we increment expected above if there is an aggregation tag.
| @@ -90,8 +90,13 @@ func newStorageWriteQuery(req *WriteQuery) (*storage.WriteQuery, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| tags := models.Tags{} | |||
There was a problem hiding this comment.
nit: Could preallocate the size here, make(models.Tags, 0, len(req.Tags))
There was a problem hiding this comment.
Yeah good call, wasn't overly concerned with the perf here since it's kind of a test endpoint but will make the change :)
| Name string `json:"name"` | ||
| Value string `json:"value"` | ||
| Name []byte `json:"name"` | ||
| Value []byte `json:"value"` |
There was a problem hiding this comment.
Just a warning, the JSON serializer will now encode these as base64:
https://play.golang.org/p/T3Xa8OGyWML
There was a problem hiding this comment.
Do matchers also need to be bytes? These only come through for reads yeah?
There was a problem hiding this comment.
Yeah, these are only needed for reads, but figured may as well convert them to bytes; would the base64 be a problem? We only really use it for sending to the session or encode them to []byte anyway on remote fetches
src/query/models/tag.go
Outdated
| } | ||
|
|
||
| return m | ||
| return []byte{}, false |
There was a problem hiding this comment.
return nil, false is slightly more optimal (no allocs)
src/query/storage/index.go
Outdated
| identTags = append(identTags, ident.StringTag(t.Name, t.Value)) | ||
| n := checked.NewBytes(t.Name, checked.NewBytesOptions()) | ||
| v := checked.NewBytes(t.Value, checked.NewBytesOptions()) | ||
| identTags = append(identTags, ident.BinaryTag(n, v)) |
There was a problem hiding this comment.
For now, perhaps we should replace with just a zero copy version (since models.Tags should be coming from the wire and won't be mutated?):
identTags := make([]ident.Tag, 0, len(tags))
for _, t := range tags {
identTags = append(identTags, ident.Tag{Name: ident.BytesID(t.Name), Value: ident.BytesID(t.Value)})
}
return ident.NewTagsIterator(ident.NewTags(identTags...)))There was a problem hiding this comment.
Oh nice, don't know why I missed the ident.Bytes; was probably looking for something like ident.ByteTag or something :p
Codecov Report
@@ Coverage Diff @@
## master #1004 +/- ##
==========================================
+ Coverage 77.59% 77.67% +0.07%
==========================================
Files 412 412
Lines 34697 34705 +8
==========================================
+ Hits 26923 26956 +33
+ Misses 5903 5880 -23
+ Partials 1871 1869 -2
Continue to review full report at Codecov.
|
src/query/storage/index.go
Outdated
| @@ -64,10 +64,13 @@ func FromIdentTagIteratorToTags(identTags ident.TagIterator) (models.Tags, error | |||
|
|
|||
| // TagsToIdentTagIterator converts coordinator tags to ident tags | |||
| func TagsToIdentTagIterator(tags models.Tags) ident.TagIterator { | |||
| //TODO get a checked bytes pool here instead | |||
There was a problem hiding this comment.
nit: Maybe update this to what remains here now that we are using ident.BytesID:
// TODO get a Tags and TagsIterator from a ident.Pool here instead of allocing them here
Change tag names and values from strings to []bytes