-
Notifications
You must be signed in to change notification settings - Fork 107
Meta tags part1: meta record data structures and corresponding CRUD api calls #1301
Conversation
adds the data structures to store meta records and also the necessary methods and api calls to insert/edit/delete them. there are also unit tests to test the meta record modifications
api/models/graphite.go
Outdated
@@ -9,7 +9,7 @@ import ( | |||
"github.com/grafana/metrictank/idx" | |||
pickle "github.com/kisielk/og-rek" | |||
opentracing "github.com/opentracing/opentracing-go" | |||
"gopkg.in/macaron.v1" | |||
macaron "gopkg.in/macaron.v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vscode in the default settings uses goreturns
to format the code on saving. that's documented here: https://github.com/Microsoft/vscode-go/wiki/On-Save-features#format-on-save
if you want I can disable that, or switch it to gofmt
which doesn't add the import name explicitly even if the package name differs from the import path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think goreturns is to blame here, something else is going on
go get -u github.com/sqs/goreturns && goreturns -w api/routes.go && grep macaron.v1 api/routes.go
"gopkg.in/macaron.v1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's strange, because i've tested that too before I wrote the last comment:
mst@mst-nb1:~/documents/code/go/src/github.com/grafana/metrictank$ cat api/models/graphite.go | grep macaron.v1
"gopkg.in/macaron.v1"
mst@mst-nb1:~/documents/code/go/src/github.com/grafana/metrictank$ gofmt api/models/graphite.go | grep macaron.v1
"gopkg.in/macaron.v1"
mst@mst-nb1:~/documents/code/go/src/github.com/grafana/metrictank$ goreturns api/models/graphite.go | grep macaron.v1
macaron "gopkg.in/macaron.v1"
I think my goreturns
has been installed by vscode. Maybe that installed a different version or default config or something like that. i'll try to figure out where this is defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after doing go get -u github.com/sqs/goreturns
it doesn't do that anymore. i guess something must have changed in the newer version. anyway, i'll undo those changes that the old version made
// The first slice of strings has the meta tags & values | ||
// The second slice has the tag query expressions which the meta tags & values refer to | ||
// On parsing error the second returned value is an error, otherwise it is nil | ||
func metaTagRecordFromStrings(metaTags []string, tagQueryExpressions []string) (metaTagRecord, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i'm missing something, but wouldn't it be cleaner to validate much earlier? typically validation happens in the rest api layer. e.g. on the models or one of the first things in the http handler. seems we're quite deep into the callpath already here with potentially invalid data, seems weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion to move the parsing/validation to an earlier stage makes sense. At the moment this is here for consistency with all the other tag query methods, such as:
metrictank/idx/memory/memory.go
Line 1012 in 947555d
func (m *UnpartitionedMemoryIdx) FindByTag(orgId uint32, expressions []string, from int64) ([]idx.Node, error) { |
It would probably make sense to move the whole expression parsing/validation for all the tag query handling methods to an earlier stage in the request handling. But I wouldn't want to do that in this PR, otherwise this PR will also need to modify a lot of the existing tag query call-paths.
record, created, err = m.MetaTagRecordUpsert(orgId, rawRecord) | ||
} else { | ||
_, _, err = m.MetaTagRecordUpsert(orgId, rawRecord) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are not closing over the m
variable correctly.
run this and then try without the m := m
line
package main
import (
"context"
"fmt"
"time"
"golang.org/x/sync/errgroup"
)
func main() {
g, _ := errgroup.WithContext(context.Background())
parts := []int{0, 1, 2, 3, 4, 5}
for _, m := range parts {
m := m
g.Go(func() error {
fmt.Println(m)
return nil
})
}
time.Sleep(time.Second)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
1) don't expose the id to the user, as this information is not useful 2) use recordId type for record ids, instead of uint32 3) rename variables that call the record id "hash" to id, also update many comments accordingly
Co-Authored-By: replay <mauro.stettler@gmail.com>
I think I've addressed every comment. @Dieterbe, please take another look when you get a chance. |
see #660 |
This is the first of multiple PRs to implement that meta tag feature.
The commits in this PR are also part of #960. I'm planning to merge the meta tag feature in multiple steps where each step is smaller and easier to review than merging the whole feature in one gigantic PR.
This PR adds the meta record data structure which keeps the definitions of the meta tags. It also adds the necessary API calls to add/upset/delete meta records. These structures will only become effective with future PRs, at the moment modifying them has no effect.