Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[query] Add start and end time to tag values endpoint #1601

Merged
merged 5 commits into from
May 4, 2019
Merged

[query] Add start and end time to tag values endpoint #1601

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec4c717
[query] Add start and end time to tag values endpoint
arnikola May 2, 2019
12a3078
PR response; adding test for tag values function
arnikola May 2, 2019
f878833
Merge branch 'master' into arnikola/fix-start-end-tag-values
arnikola May 2, 2019
54a359b
Lint fixes
arnikola May 2, 2019
89fa16f
Merge branch 'master' into arnikola/fix-start-end-tag-values
May 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/query/api/v1/handler/prometheus/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add test to catch this please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm but it wasn't working before? so we should probably have a test to make sure it doesn't break in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These weren't being set; won't really be a useful test

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm maybe add an e2e test instead of a unit test, something emulating the behaviour that was failing for users but isn't now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like ideally, that test fails without this PR and works with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah refactored it a bit to make it nicer and added a couple of tests

CompleteNameOnly: false,
FilterNameTags: [][]byte{nameBytes},
TagMatchers: models.Matchers{
Expand Down