-
Notifications
You must be signed in to change notification settings - Fork 107
Add /tags/terms
query to get counts of tag values
#1582
Conversation
Example:
|
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.
looks pretty good, though i didn't go to deep in the index. i'll defer to mauro or robert on that.
// 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) |
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 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.
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.
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.
idx/memory/memory.go
Outdated
m.RLock() | ||
defer m.RUnlock() | ||
|
||
// TODO - terms for meta tags? |
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.
let's make up our mind. is it something to do (when?), or is this something we're deciding not to support for now?
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.
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?
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.
How much work would it be to enrich here? cc @replay
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.
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)) |
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 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?
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. |
Aside from the other comments it looks fine. |
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. |
ok so let's not do meta tags then yet. |
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.
splendid
If the interface / behavior is approved, I'll update the HTTP docs.
Fix #1573