Griesbacher / histou

Adds templates to Grafana in combination with nagflux
GNU General Public License v2.0
35 stars 16 forks source link

Additional default config for influxdb queries #21

Closed b1zzu closed 6 years ago

b1zzu commented 6 years ago

Hi, We have work on crate two new default options for all influxdb queries. We need this two options because we want to set them for all graph without the need to change all templates, but always allow templates to overwrite them.

The two options are:

These two options are fully descript in the README.md, and for both options, we have created a unittest.

If these two options are not set, nothing will change from the previous behavior.

Bye, Davide

Griesbacher commented 6 years ago

Hi Davide,

thanks for the PR. I'm not so happy, how you solved the problem, because it's very static. You can only change your values via config file, which applies them to every graph. In my opinion this should be done in a template or per URL arguments. Your fill options can be added to a target after the creation and do not to have to take place in the construction. Like the following pseudo code:

public function addGroupByTimeToTarget($target, $GroupByTime='5m'){
    $target + $GroupByTime;
    return $target;
}

public function addGroupByTimeFillToTarget($target, $GroupByTimeFill='previous'){
    $target + $GroupByTimeFill;
    return $target;
}

In some way similar to these: https://github.com/WuerthPhoenix/histou/blob/54a96486981727916c03f64d222037dbe920267c/histou/grafana/graphpanel/graphpanelinfluxdb.php#L131-L137

This would eliminate the need of new config file options, createTarget would not change, no previous code has to change and it would lead to a complete backward compatibility.

Best wishes, Philip

b1zzu commented 6 years ago

Hi Philip, I totally understand your point of view. But we need to spread this options automatically to all templates without change all templates.

I don't see another way to integrate these requirements in a non-static way and that doesn't look like a workaround. So for now, we will keep this patch in our fork.

Thank for your time.

Bye, Davide