Add `/tags/terms` query to get counts of tag values by shanson7 · Pull Request #1582 · grafana/metrictank

shanson7 · 2019-12-27T17:29:56Z

If the interface / behavior is approved, I'll update the HTTP docs.

shanson7 · 2019-12-27T17:32:28Z

Example:

curl -s 'http://localhost:6060/tags/terms?expr=namespace=ossh&tags=name'
{
  "totalSeries": 15387,
  "terms": {
    "name": {
      "disk.average.latency.g": 925,
      "disk.average.service.time.g": 927,
      "disk.io.current.g": 961,
      "disk.io.time.g": 875,
      "disk.io.time.weighted.g": 928,
      "disk.reads.completed.g": 921,
      "disk.reads.merged.g": 946,
      "disk.reads.time.g": 985,
      "disk.sectors.read.g": 963,
      "disk.sectors.written.g": 921,
      "disk.writes.completed.g": 970,
      "disk.writes.merged.g": 924,
      "disk.writes.time.g": 953,
      "interrupt.g": 553,
      "network.softnet.cpu.collisions.g": 848,
      "network.softnet.cpu.packets.dropped.g": 899,
      "network.softnet.cpu.time.squeeze.g": 888
    }
  }
}

Dieterbe

looks pretty good, though i didn't go to deep in the index. i'll defer to mauro or robert on that.

idx/idx.go

Dieterbe · 2020-01-06T20:58:27Z

idx/idx.go

+	// is kept for each value of the requested tags.
+	// The series are not deduplicated and in certain cases it is possible that some
+	// entries will be double counted.
+	FindTerms(orgID uint32, tags []string, query tagquery.Query) (uint32, map[string]map[string]uint32)


maybe personal preference, but i think elsewhere we somewhat try to order args by the order in which they're used.
if we first execute the query, then look at its result based on tags, we should probably put tags after the query.

Looking at the other functions, this query seems to come later. In fact, I actually do use tags first (meaningfully, at least). I switched the order of use in the implementation, so §tags§ is used first for real.

Dieterbe · 2020-01-06T21:00:03Z

idx/memory/memory.go

+	m.RLock()
+	defer m.RUnlock()
+
+	// TODO - terms for meta tags?


let's make up our mind. is it something to do (when?), or is this something we're deciding not to support for now?

Yes, this TODO was meant to spark discussion (forgot to include it when entering the PR). We don't have a need for meta tags in this spot, but maybe it should be supported anyway?

How much work would it be to enrich here? cc @replay

would be easy and relatively fast, basically just this needs to be done:

metrictank/idx/memory/memory.go

Line 1095 in 1735500

byPath[nameWithTags].MetaTags = mtr.getMetaTagsByRecordIds(enricher.enrich(def.Id.Key))

I think this is the only remaining point. I'm not entirely sure when we would want to include meta tags for this endpoint, since, as @robert-milan mentions, meta tags are really just tied to other tags (albeit in a non-trivial way). Does anyone have any strong preference?

idx/memory/partitioned_idx.go

api/routes.go

replay · 2020-01-09T14:15:43Z

Looks good to me. Once we have a conclusion regarding whether meta tags should be included in the result it's fine for me to merge it.

robert-milan · 2020-01-10T13:37:24Z

Aside from the other comments it looks fine.

robert-milan · 2020-01-10T13:39:08Z

Looks good to me. Once we have a conclusion regarding whether meta tags should be included in the result it's fine for me to merge it.

I feel like meta tags is a separate concept and should not be included in the results. Since meta tags themselves are just collections of tags I think it might confuse things a bit.

Dieterbe · 2020-01-14T18:52:45Z

ok so let's not do meta tags then yet.
when docs are updated, we can merge.

Dieterbe

splendid

Dieterbe suggested changes Jan 6, 2020

View reviewed changes

replay reviewed Jan 9, 2020

View reviewed changes

api/routes.go Outdated Show resolved Hide resolved

robert-milan approved these changes Jan 10, 2020

View reviewed changes

shanson7 added 4 commits January 15, 2020 13:51

Add types for terms query

a58084f

Add tags/terms implementation

dc3ad20

Small comment cleanups

840ed9b

Add docs

8707cbb

shanson7 force-pushed the terms branch from eeb4ad5 to 8707cbb Compare January 15, 2020 14:22

robert-milan requested a review from Dieterbe January 17, 2020 01:49

Dieterbe approved these changes Jan 20, 2020

View reviewed changes

Dieterbe merged commit 02376a7 into grafana:master Jan 20, 2020

shanson7 deleted the terms branch January 21, 2020 11:06

Conversation

shanson7 commented Dec 27, 2019

Uh oh!

shanson7 commented Dec 27, 2019

Uh oh!

Dieterbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dieterbe Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

shanson7 Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

Dieterbe Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

shanson7 Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

Dieterbe Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

replay Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

shanson7 Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

replay commented Jan 9, 2020

Uh oh!

robert-milan commented Jan 10, 2020

Uh oh!

robert-milan commented Jan 10, 2020

Uh oh!

Dieterbe commented Jan 14, 2020

Uh oh!

Dieterbe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants