Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented the tag values iterator for SHOW TAG VALUES #5853

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

jsternberg
Copy link
Contributor

Fixes #5593.

@jsternberg
Copy link
Contributor Author

I had to add another filtering function which was a bit weird and I would prefer another solution with more code reuse, but I couldn't think of a way that didn't break another part of functionality.

@jsternberg jsternberg changed the title Implemented the tag values iterator for SHOW TAG VALUES [WIP] Implemented the tag values iterator for SHOW TAG VALUES Feb 26, 2016
@jsternberg
Copy link
Contributor Author

Not quite done yet. It still needs to handle more functionality that I didn't realize needed to be covered. This kind of call has to be supported: SHOW TAG VALUES WITH KEY = host WHERE region =~ /ca.*/

@jsternberg jsternberg changed the title [WIP] Implemented the tag values iterator for SHOW TAG VALUES Implemented the tag values iterator for SHOW TAG VALUES Mar 4, 2016
@jsternberg
Copy link
Contributor Author

I think it's working now.

@jwilder @e-dard @benbjohnson please review. I'm not sure how happy I am with the end result, but I do believe it works. This change also affects backwards compatibility so I would like any feedback regarding that. The change to the output is located in the commit message.

n.Condition = cond.(Expr)
} else {
n.Condition = nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make a RewriteExpr() that accepts and returns an Expr so that you don't have to do the nil cast everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a good idea. I can do that.

@benbjohnson
Copy link
Contributor

Overall it lgtm. Can you post a before/after example of the output and verify with @pauldix about the change. I'm ok with it.

`SHOW TAG VALUES` output has been modified to print the measurement name
for every measurement and to return the output in two columns: key and
value. An example output might be:

    > SHOW TAG VALUES WITH KEY IN (host, region)
    name: cpu
    ---------
    key     value
    host    server01
    region  useast

    name: mem
    ---------
    key     value
    host    server02
    region  useast

`measurementsByExpr` has been taught how to handle reserved keys (ones
with an underscore at the beginning) to allow reusing that function and
skipping over expressions that don't matter to the call.

Fixes #5593.
@jsternberg
Copy link
Contributor Author

Change in output from:

> SHOW TAG VALUES WITH KEY IN (host, region)
name: hostTagValues
-------------------
host
server01
server02

name: regionTagValues
---------------------
region
useast

to

> SHOW TAG VALUES WITH KEY IN (host, region)
name: cpu
---------
key     value
host    server01
region  useast

name: mem
---------
key     value
host    server02
region  useast

@pauldix
Copy link
Member

pauldix commented Mar 6, 2016

new output format looks good to me. Does it support SHOW TAG VALUES FROM cpu WITH KEY IN (host, region)?

@mark-rushakoff: this change will almost certainly break Chronograf so we'll have to update for it.

@jsternberg
Copy link
Contributor Author

@pauldix yep, it supports that.

> insert cpu,host=server01 value=2
> insert mem,host=server02 value=3
> show tag values from cpu with key = host
name: cpu
---------
key     value
host    server01

> show tag values with key = host
name: cpu
---------
key     value
host    server01


name: mem
---------
key     value
host    server02

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants