[query] Add start and end time to tag values endpoint #1601
Conversation
| @@ -241,6 +241,9 @@ func ParseTagValuesToQuery( | |||
|
|
|||
| nameBytes := []byte(name) | |||
| return &storage.CompleteTagsQuery{ | |||
| // NB: necessarily spans the entire timerange for the index. | |||
| Start: time.Time{}, | |||
| End: time.Now(), | |||
There was a problem hiding this comment.
Can you add test to catch this please?
There was a problem hiding this comment.
It's just setting variables for a struct; since it uses this weird gorilla mux, it's not really plausible to generate a request capable of being read by it easily
There was a problem hiding this comment.
hmm but it wasn't working before? so we should probably have a test to make sure it doesn't break in the future?
There was a problem hiding this comment.
These weren't being set; won't really be a useful test
There was a problem hiding this comment.
hm maybe add an e2e test instead of a unit test, something emulating the behaviour that was failing for users but isn't now?
There was a problem hiding this comment.
like ideally, that test fails without this PR and works with it.
There was a problem hiding this comment.
Yeah refactored it a bit to make it nicer and added a couple of tests
| @@ -25,6 +25,8 @@ import ( | |||
| "net/http" | |||
| "time" | |||
|
|
|||
| "github.com/m3db/m3/src/x/clock" | |||
|
|
|||
There was a problem hiding this comment.
nit: Remove superfluous endline.
There was a problem hiding this comment.
Will do; it's odd that the linter didn't fail here
Codecov Report
@@ Coverage Diff @@
## master #1601 +/- ##
======================================
Coverage 71.7% 71.7%
======================================
Files 953 953
Lines 78954 78954
======================================
Hits 56661 56661
Misses 18562 18562
Partials 3731 3731
Continue to review full report at Codecov.
|
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type tagValuesMatcher struct { |
There was a problem hiding this comment.
more a FYI for future - can use google's compare method as a matcher for most things - it usually does the right thing: compare struct fields one at a time/use Equal() if it exists on a type/etc. There's a wrapper for it here: https://github.com/m3db/m3/blob/master/src/x/test/matcher.go
Fixes an issue where the values endpoint,
/label/{name}/valueswas not returning correct results. Issue was caused since the new optimized tag completion endpoint now considers query range params, and they were not being set correctly.Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: