Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

public orgId -1 -> 0 and related cleanups #880

Merged
merged 4 commits into from
Apr 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions docs/http-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ POST /metrics/index.json

* header `X-Org-Id` required

Returns metrics stored under the given org, as well as public data under org -1 (see [multi-tenancy](https://github.com/grafana/metrictank/blob/master/docs/multi-tenancy.md))
If orgId is -1, returns the metrics for all orgs. (but you can't necessarily distinguish which org a metric is from)
Returns metrics stored under the given org, as well as public data under org 0 (see [multi-tenancy](https://github.com/grafana/metrictank/blob/master/docs/multi-tenancy.md))

#### Example

Expand All @@ -57,7 +56,7 @@ POST /metrics/find
* format: json, treejson, completer, pickle, or msgpack. (defaults to json)
* jsonp

Returns metrics which match the query and are stored under the given org or are public data under org -1 (see [multi-tenancy](https://github.com/grafana/metrictank/blob/master/docs/multi-tenancy.md))
Returns metrics which match the query and are stored under the given org or are public data under org 0 (see [multi-tenancy](https://github.com/grafana/metrictank/blob/master/docs/multi-tenancy.md))
the completer format is for completion UI's such as graphite-web.
json and treejson are the same.

Expand Down Expand Up @@ -113,7 +112,7 @@ POST /render

If metrictank doesn't have a requested function, it always proxies to graphite, irrespective of this setting.

Data queried for must be stored under the given org or be public data under org -1 (see [multi-tenancy](https://github.com/grafana/metrictank/blob/master/docs/multi-tenancy.md))
Data queried for must be stored under the given org or be public data under org 0 (see [multi-tenancy](https://github.com/grafana/metrictank/blob/master/docs/multi-tenancy.md))

#### Example

Expand Down
2 changes: 1 addition & 1 deletion docs/multi-tenancy.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ A metrictank based stack is multi-tenant. Here's how it works:
* For a secure setup, you must make sure these headers cannot be specified by users. You may need to run something in front to set the header correctly after authentication
(e.g. [tsdb-gw](https://github.com/raintank/tsdb-gw)
* orgs can only see the data that lives under their org-id, and also public data
* public data is stored under orgId -1 and is visible to everyone.
* public data is stored under orgId 0 and is visible to everyone.
6 changes: 3 additions & 3 deletions idx/cassandra/cassandra.go
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,9 @@ func (c *CasIdx) deleteDef(def *idx.Archive) error {
return fmt.Errorf("unable to delete metricDef %s from index after %d attempts.", def.Id, attempts)
}

func (c *CasIdx) Prune(orgId int, oldest time.Time) ([]idx.Archive, error) {
func (c *CasIdx) Prune(oldest time.Time) ([]idx.Archive, error) {
pre := time.Now()
pruned, err := c.MemoryIdx.Prune(orgId, oldest)
pruned, err := c.MemoryIdx.Prune(oldest)
statPruneDuration.Value(time.Since(pre))
return pruned, err
}
Expand All @@ -504,7 +504,7 @@ func (c *CasIdx) prune() {
for range ticker.C {
log.Debug("cassandra-idx: pruning items from index that have not been seen for %s", maxStale.String())
staleTs := time.Now().Add(maxStale * -1)
_, err := c.Prune(-1, staleTs)
_, err := c.Prune(staleTs)
if err != nil {
log.Error(3, "cassandra-idx: prune error. %s", err)
}
Expand Down
6 changes: 3 additions & 3 deletions idx/cassandra/cassandra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func TestGetAddKey(t *testing.T) {
ix := New()
initForTests(ix)

publicSeries := getMetricData(-1, 2, 5, 10, "metric.public")
publicSeries := getMetricData(idx.OrgIdPublic, 2, 5, 10, "metric.public")
org1Series := getMetricData(1, 2, 5, 10, "metric.org1")
org2Series := getMetricData(2, 2, 5, 10, "metric.org2")

Expand All @@ -138,7 +138,7 @@ func TestGetAddKey(t *testing.T) {
Convey(fmt.Sprintf("Then listing metrics for OrgId %d", orgId), func() {
defs := ix.List(orgId)
numSeries := len(series)
if orgId != -1 {
if orgId != idx.OrgIdPublic {
numSeries += 5
}
So(defs, ShouldHaveLength, numSeries)
Expand Down Expand Up @@ -258,7 +258,7 @@ func TestAddToWriteQueue(t *testing.T) {
func TestFind(t *testing.T) {
ix := New()
initForTests(ix)
for _, s := range getMetricData(-1, 2, 5, 10, "metric.demo") {
for _, s := range getMetricData(idx.OrgIdPublic, 2, 5, 10, "metric.demo") {
ix.AddOrUpdate(s, 1)
}
for _, s := range getMetricData(1, 2, 5, 10, "metric.demo") {
Expand Down
12 changes: 6 additions & 6 deletions idx/idx.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
schema "gopkg.in/raintank/schema.v1"
)

const OrgIdPublic = 0
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we just make this configurable with a command line arg?

Copy link
Contributor Author

@Dieterbe Dieterbe Apr 4, 2018

Choose a reason for hiding this comment

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

what's the benefit of that?
I think this should be a constant across the entire stack and the surrounding ecosystem of tools:
org 0 = public data
org 1 = admin org
org >1 = private tenant orgs

making it configurable seems to cause nothing but complications.

Copy link
Member

Choose a reason for hiding this comment

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

Why does the public org need to be 0. Wasnt the whole point of using a variable (or const) to remove the need to have special values.

For the Wolrdping case, we currently have to hard code the collection of the public data into the probe. But we could simplify everything by just creating a public org and configure the endpoints for it, known that the data collected for that org will be viewable by all users.

Copy link
Contributor Author

@Dieterbe Dieterbe Apr 4, 2018

Choose a reason for hiding this comment

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

Wasnt the whole point of using a variable (or const) to remove the need to have special values.

No, the point was to only use orgid's that are >=0 so that they are encodeable as a uint. this is also what was explained in the original ticket #260

there's nothing wrong with special reserved values per se. I think conventions are useful, as long as they are simple and consistently applied. they simplify things (less config flags to worry about and keep in sync across different software, clearer code)

Your proposed simplification for WP probes sounds nice, but does it really conflict with the proposed convention? is it not possible to just create that public org with id 0 ?

Copy link
Member

Choose a reason for hiding this comment

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

I dont really like using the zero value, as it then makes it impossible to know if the value was set or not.

But as we dont have a use case for needing non-zero right now, lets just leave this. It will be trivial to change this later if we want/need to.


var (
BothBranchAndLeaf = errors.New("node can't be both branch and leaf")
BranchUnderLeaf = errors.New("can't add branch under leaf")
Expand Down Expand Up @@ -118,20 +120,18 @@ type MetricIndex interface {

// Find searches the index. The method is passed an OrgId, a query
// pattern and a unix timestamp. Searches should return all nodes that match for
// the given OrgId and OrgId -1. The pattern should be handled in the same way
// the given OrgId and OrgIdPublic. The pattern should be handled in the same way
// Graphite would. see https://graphite.readthedocs.io/en/latest/render_api.html#paths-and-wildcards
// And the unix stimestamp is used to ignore series that have been stale since
// the timestamp.
Find(int, string, int64) ([]Node, error)

// List returns all Archives for the passed OrgId, or for all organisations if -1 is provided.
// List returns all Archives for the passed OrgId and the public orgId
List(int) []Archive

// Prune deletes all metrics from the index for the passed org where
// the last time the metric was seen is older then the passed timestamp. If the org
// passed is -1, then the all orgs should be examined for stale metrics to be deleted.
// Prune deletes all metrics that haven't been seen since the given timestamp.
// It returns all Archives deleted and any error encountered.
Prune(int, time.Time) ([]Archive, error)
Prune(time.Time) ([]Archive, error)

// FindByTag takes a list of expressions in the format key<operator>value.
// The allowed operators are: =, !=, =~, !=~.
Expand Down
61 changes: 19 additions & 42 deletions idx/memory/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -836,15 +836,17 @@ func (m *MemoryIdx) Find(orgId int, pattern string, from int64) ([]idx.Node, err
if err != nil {
return nil, err
}
publicNodes, err := m.find(-1, pattern)
if err != nil {
return nil, err
if orgId != idx.OrgIdPublic {
publicNodes, err := m.find(idx.OrgIdPublic, pattern)
if err != nil {
return nil, err
}
matchedNodes = append(matchedNodes, publicNodes...)
}
matchedNodes = append(matchedNodes, publicNodes...)
log.Debug("memory-idx: %d nodes matching pattern %s found", len(matchedNodes), pattern)
results := make([]idx.Node, 0)
seen := make(map[string]struct{})
// if there are public (orgId -1) and private leaf nodes with the same series
// if there are public (orgId OrgIdPublic) and private leaf nodes with the same series
// path, then the public metricDefs will be excluded.
for _, n := range matchedNodes {
if _, ok := seen[n.Path]; !ok {
Expand Down Expand Up @@ -977,26 +979,11 @@ func (m *MemoryIdx) List(orgId int) []idx.Archive {
m.RLock()
defer m.RUnlock()

orgs := make(map[int]struct{})
if orgId == -1 {
log.Info("memory-idx: returning all metricDefs for all orgs")
for org := range m.tree {
orgs[org] = struct{}{}
}
for org := range m.tags {
orgs[org] = struct{}{}
}
} else {
orgs[-1] = struct{}{}
orgs[orgId] = struct{}{}
}

defs := make([]idx.Archive, 0)
for _, def := range m.defById {
if _, ok := orgs[def.OrgId]; !ok {
continue
if def.OrgId == orgId || def.OrgId == idx.OrgIdPublic {
defs = append(defs, *def)
}
defs = append(defs, *def)
}

statListDuration.Value(time.Since(pre))
Expand Down Expand Up @@ -1187,24 +1174,20 @@ func (m *MemoryIdx) delete(orgId int, n *Node, deleteEmptyParents, deleteChildre
}

// delete series from the index if they have not been seen since "oldest"
func (m *MemoryIdx) Prune(orgId int, oldest time.Time) ([]idx.Archive, error) {
func (m *MemoryIdx) Prune(oldest time.Time) ([]idx.Archive, error) {
oldestUnix := oldest.Unix()
orgs := make(map[int]struct{})
if orgId == -1 {
log.Info("memory-idx: pruning stale metricDefs across all orgs")
m.RLock()
for org := range m.tree {
log.Info("memory-idx: pruning stale metricDefs across all orgs")
m.RLock()
for org := range m.tree {
orgs[org] = struct{}{}
}
if TagSupport {
for org := range m.tags {
orgs[org] = struct{}{}
}
if TagSupport {
for org := range m.tags {
orgs[org] = struct{}{}
}
}
m.RUnlock()
} else {
orgs[orgId] = struct{}{}
}
m.RUnlock()

var pruned []idx.Archive
toPruneUntagged := make(map[int]map[string]struct{}, len(orgs))
Expand All @@ -1218,10 +1201,6 @@ func (m *MemoryIdx) Prune(orgId int, oldest time.Time) ([]idx.Archive, error) {
m.RLock()
DEFS:
for _, def := range m.defById {
if _, ok := orgs[def.OrgId]; !ok {
continue DEFS
}

if def.LastUpdate >= oldestUnix {
continue DEFS
}
Expand Down Expand Up @@ -1300,9 +1279,7 @@ DEFS:

statMetricsActive.Add(-1 * len(pruned))

if orgId == -1 {
log.Info("memory-idx: pruning stale metricDefs from memory for all orgs took %s", time.Since(pre).String())
}
log.Info("memory-idx: pruning stale metricDefs from memory for all orgs took %s", time.Since(pre).String())

statPruneDuration.Value(time.Since(pre))
return pruned, nil
Expand Down
30 changes: 15 additions & 15 deletions idx/memory/memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func testGetAddKey(t *testing.T) {
ix := New()
ix.Init()

publicSeries := getMetricData(-1, 2, 5, 10, "metric.public", false)
publicSeries := getMetricData(idx.OrgIdPublic, 2, 5, 10, "metric.public", false)
org1Series := getMetricData(1, 2, 5, 10, "metric.org1", false)
org2Series := getMetricData(2, 2, 5, 10, "metric.org2", false)

Expand All @@ -99,7 +99,7 @@ func testGetAddKey(t *testing.T) {
Convey(fmt.Sprintf("Then listing metrics for OrgId %d", orgId), func() {
defs := ix.List(orgId)
numSeries := len(series)
if orgId != -1 {
if orgId != idx.OrgIdPublic {
numSeries += 5
}
So(defs, ShouldHaveLength, numSeries)
Expand Down Expand Up @@ -128,7 +128,7 @@ func TestFind(t *testing.T) {
func testFind(t *testing.T) {
ix := New()
ix.Init()
for _, s := range getMetricData(-1, 2, 5, 10, "metric.demo", false) {
for _, s := range getMetricData(idx.OrgIdPublic, 2, 5, 10, "metric.demo", false) {
s.Time = 10 * 86400
ix.AddOrUpdate(s, 1)
}
Expand Down Expand Up @@ -256,7 +256,7 @@ func testDelete(t *testing.T) {
ix := New()
ix.Init()

publicSeries := getMetricData(-1, 2, 5, 10, "metric.public", false)
publicSeries := getMetricData(idx.OrgIdPublic, 2, 5, 10, "metric.public", false)
org1Series := getMetricData(1, 2, 5, 10, "metric.org1", false)

for _, s := range publicSeries {
Expand All @@ -271,7 +271,7 @@ func TestDeleteTagged(t *testing.T) {
ix := New()
ix.Init()

publicSeries := getMetricData(-1, 2, 5, 10, "metric.public", true)
publicSeries := getMetricData(idx.OrgIdPublic, 2, 5, 10, "metric.public", true)
org1Series := getMetricData(1, 2, 5, 10, "metric.org1", true)

for _, s := range publicSeries {
Expand Down Expand Up @@ -506,12 +506,12 @@ func TestPruneTaggedSeries(t *testing.T) {
}

Convey("after populating index", t, func() {
defs := ix.List(-1)
defs := ix.List(1)
So(defs, ShouldHaveLength, 10)
})

Convey("When purging old series", t, func() {
purged, err := ix.Prune(1, time.Unix(2, 0))
purged, err := ix.Prune(time.Unix(2, 0))
So(err, ShouldBeNil)
So(purged, ShouldHaveLength, 5)
nodes, err := ix.FindByTag(1, []string{"name=~metric\\.bah.*", "series_id=~[0-4]"}, 0)
Expand All @@ -523,7 +523,7 @@ func TestPruneTaggedSeries(t *testing.T) {
})

Convey("after purge", t, func() {
defs := ix.List(-1)
defs := ix.List(1)
So(defs, ShouldHaveLength, 5)
data := &schema.MetricData{
Name: defs[0].Name,
Expand All @@ -538,7 +538,7 @@ func TestPruneTaggedSeries(t *testing.T) {
data.SetId()
ix.AddOrUpdate(data, 1)
Convey("When purging old series", func() {
purged, err := ix.Prune(1, time.Unix(12, 0))
purged, err := ix.Prune(time.Unix(12, 0))
So(err, ShouldBeNil)
So(purged, ShouldHaveLength, 4)
nodes, err := ix.FindByTag(1, []string{"name=~metric\\.foo.*", "series_id=~[0-4]"}, 0)
Expand Down Expand Up @@ -581,7 +581,7 @@ func TestPruneTaggedSeriesWithCollidingTagSets(t *testing.T) {
}

Convey("When purging old series", t, func() {
purged, err := ix.Prune(1, time.Unix(2, 0))
purged, err := ix.Prune(time.Unix(2, 0))
So(err, ShouldBeNil)
So(purged, ShouldHaveLength, 0)
})
Expand All @@ -593,7 +593,7 @@ func TestPruneTaggedSeriesWithCollidingTagSets(t *testing.T) {
})

Convey("When purging newer series", t, func() {
purged, err := ix.Prune(1, time.Unix(20, 0))
purged, err := ix.Prune(time.Unix(20, 0))
So(err, ShouldBeNil)
So(purged, ShouldHaveLength, 2)
})
Expand Down Expand Up @@ -638,11 +638,11 @@ func testPrune(t *testing.T) {
ix.AddOrUpdate(d, 1)
}
Convey("after populating index", t, func() {
defs := ix.List(-1)
defs := ix.List(1)
So(defs, ShouldHaveLength, 10)
})
Convey("When purging old series", t, func() {
purged, err := ix.Prune(1, time.Unix(2, 0))
purged, err := ix.Prune(time.Unix(2, 0))
So(err, ShouldBeNil)
So(purged, ShouldHaveLength, 5)
nodes, err := ix.Find(1, "metric.bah.*", 0)
Expand All @@ -654,7 +654,7 @@ func testPrune(t *testing.T) {

})
Convey("after purge", t, func() {
defs := ix.List(-1)
defs := ix.List(1)
So(defs, ShouldHaveLength, 5)
data := &schema.MetricData{
Name: defs[0].Name,
Expand All @@ -667,7 +667,7 @@ func testPrune(t *testing.T) {
data.SetId()
ix.AddOrUpdate(data, 0)
Convey("When purging old series", func() {
purged, err := ix.Prune(1, time.Unix(12, 0))
purged, err := ix.Prune(time.Unix(12, 0))
So(err, ShouldBeNil)
So(purged, ShouldHaveLength, 4)
nodes, err := ix.Find(1, "metric.foo.*", 0)
Expand Down