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

mt-index-cat #437

Merged
merged 6 commits into from
Dec 27, 2016
Merged

mt-index-cat #437

merged 6 commits into from
Dec 27, 2016

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Dec 24, 2016

this tool aims to make it easy to see what's in the index, though the most common use is the vegeta output format which lets you generate queries to target different series on metrictank, so you can pipe the output into vegeta attack

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

just comments, nothing critical

var from string
var maxAge string
var count bool

Copy link
Contributor

Choose a reason for hiding this comment

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

needless empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i use it to separate variables for flags from other internal vars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll just move the interval var down to where its used

flag.Usage = func() {
fmt.Println(os.Args[0])
fmt.Printf("Usage:\n\n")
fmt.Printf(" inspect-idx [global config flags] <idxtype> [idx config flags] output \n\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the name of the executable be mt-index-cat now?

os.Exit(-1)
}

var show func(d schema.MetricDefinition)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this declaration down so it's right above the switch on line 99 where it gets used. Just to save the reader some scrolling.

var found bool
for _, output := range outputs {
if last == output {
found = true
Copy link
Contributor

Choose a reason for hiding this comment

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

A break would stop the loop once and output is found.

case "vegeta-render-patterns":
show = out.GetVegetaRenderPattern(addr, from)
default:
panic("this should never happen. we already validated the output type")
Copy link
Contributor

Choose a reason for hiding this comment

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

That's indeed never supposed to happen, and the message also doesn't provide much information, so why even having the default case?
Actually I'd probably type output func(schema.MetricDefinition) and build a map like map[string]output.
Or maybe to avoid unnecessary calls to external packages even like that:

type output func()(func(schema.MetricDefinition))
outputs["vegeta-render"] = func() (func(schema.MetricDefinition)) {return out.GetVegetaRender(addr, from)}

That way validation if a string is in the keys of the map and the map lookup to the right function both become simple and without any ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought of this but it just gets a bit too complicated for my taste. especially as you need to know the values of addr and from, which you only get after parsing the args, so you either use closures which take those as args (extra complication) or you construct the map after you know the values, which means you can't get the list of the keys in the help message, so you still need a separate listing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably have gone for the closures because it still feels more "elegant" than having a default case that should never happen. But yeah, it's really more complicated and not important at all

defs = idx.Load(defs)

for _, d := range defs {
if maxAgeInt != 0 && d.LastUpdate > time.Now().Unix()-maxAgeInt {
Copy link
Contributor

Choose a reason for hiding this comment

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

if maxAgeInt == 0 we do not need to enter this whole loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually this made me realize there's a bug here. if maxAgeInt is 0, it means we should show them all without filtering.

var orgId int
var partition int32
var lastupdate int64
var interval int
Copy link
Contributor

Choose a reason for hiding this comment

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

if string type variables are consolidated on a single line then the int type ones could be on one line too

@Dieterbe Dieterbe changed the title Mt index cat mt-index-cat Dec 26, 2016
Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

still looks good

@Dieterbe Dieterbe merged commit baf2e47 into master Dec 27, 2016
Dieterbe added a commit that referenced this pull request Jan 2, 2017
Dieterbe added a commit that referenced this pull request Jan 2, 2017
@Dieterbe Dieterbe deleted the mt-index-cat branch December 15, 2017 22:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants