vimeo / graph-explorer

A graphite dashboard powered by structured metrics
http://vimeo.github.io/graph-explorer/
Apache License 2.0
1.06k stars 93 forks source link

Implement avgby. #33

Closed zehome closed 11 years ago

zehome commented 11 years ago

Fixes #30

Dieterbe commented 11 years ago

looks good. other than that one thing i commented in the code. i like how the code modifies graph_config instead of graphs[graph_key]

Dieterbe commented 11 years ago

great work. as you can see i did a few extra smaller commits.

Dieterbe commented 11 years ago

i pushed some more things, including that check for tags appearing in more then one of avg/sum/group by. have a look at my commits and let me know what you think.

could you also explain why you need a copy @

all_targets = graph_config['targets'][:]  # Get a copy
zehome commented 11 years ago

Sure,

because of this line:

+                    for t in targets:
+                        all_targets.remove(t)

In python if you do:

for i in mylist:
  mylist.remove(x)

You NEED to get a copy of mylist or the forloop will go crazy. So you do a copy of the list before iterating on it :)

Dieterbe commented 11 years ago

but that targets variable is actually coming from

for (avg_id, targets) in graph_config['targets_avg_candidates'].items():

not from graph_config['targets'] so it's a different list

zehome commented 11 years ago

That's right, anyway the code don't use/remove stuff from all_targets from the same loop, so it should be safe to use directly. It's probably overkill to get a copy so.