This repository has been archived by the owner on Aug 23, 2023. It is now read-only.
Promql integration v2 #858
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
for consistency and also that's how Go works
* organize into separate file * clean DRY between constructor and Querier interface method for promql * use constructor where possible, meaning we can pass orgId directly instead of as context value and we know there's no error to check * fix ns->second conversion * pass on real context instead of a background one
also tweak some errors
Dieterbe
force-pushed
the
promql-integration2
branch
from
36e2bfd
to
fbb7662
Compare
also: * newer version of dep uses multi-line format * it auto-added a bunch of constraints * needed to pin gocql. it was tricky to determine which version our gocql is supposed to be at the last update was #778 but we don't know what version of gocql that was exactly. None of the versions in gocql's last year of git history matches what we have in our vendor dir, in fact, the smallest diff with any version was still about 480 lines; so it looks like not all go files were copied over. however it seems likely it would have been d9815cdf0ff24e2efa9b8062f4f94a6dd347ae51 because our vendor dir does include that change, not some of the later changes, and the time works with that PR.
Dieterbe
force-pushed
the
promql-integration2
branch
from
fbb7662
to
788f3d5
Compare
We are getting:
```
go vet ./...
api/models/prometheus.go:88: method Seek(t int64) bool should have signature Seek(int64, int) (int64, error)
exit status 1
```
because go vet is too eagerly assuming we're trying to implement
io.Seeker even though our Seek method is not used as such.
Note that `go vet` complains about this code as well, reproducing the issue in isolation:
```
package main
import "fmt"
type A struct {
}
func main() {
a := A{}
a.Seek(42)
}
func (a *A) Seek(t int64) bool {
fmt.Println("this is my seek function", t)
return true
}
```
Note, go vet's official description is:
""
Vet examines Go source code and reports suspicious constructs, such as
Printf calls whose arguments do not align with the format string. Vet
uses heuristics that do not guarantee all reports are genuine problems,
but it can find errors not caught by the compilers.
"""
i.e. it is expected to occasionally result in false positives.
Note also, from https://golang.org/doc/go1.10#test-vet :
The go test command now automatically runs go vet on the package being
tested, to identify significant problems before running the test. Any
such problems are treated like build errors and prevent execution of the
test. Only a high-confidence subset of the available go vet checks are
enabled for this automatic check.
Therefore, we should stop relying on the full `go vet` with false positives
and instead we can simply leverage what's built into go test
|
I found the reason for all these irrelevant test flags furthermore, metrictank, mt-store-cat, and mt-whisper-importer-reader import github.com/grafana/metrictank/api so those programs get all the irrelevant test flags. |
it's different on every system. in particular, it causes scripts/qa/docs.sh to complain about bogus diffs
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@jtlisi I want to merge prometheus support asap so I went ahead and addressed all my comments in #852 myself
this comes with a bunch of new commits and also i redid the "update dependencies" commit to not introduce 70MB of unneeded stuff as explained in #852 (comment)
in fact, by pinning gocql to what I think it was supposed to be (this was hard as the last update of our vendored copy of gocql update left out some files and didn't specify a revision) I was able to avoid updating gocql and keep more stuff out of vendor and out of the git repo.
I propose we merge this PR instead of #852
please have a look in general, and in specific: