numberly / flask-graphite

:chart_with_upwards_trend: Push useful metrics for each request without effort!
https://flask-graphite.readthedocs.io
MIT License
16 stars 1 forks source link

Endpoint path separator is not supported by Graphite web #16

Open shapiy opened 5 years ago

shapiy commented 5 years ago

First of all, thanks for implementing such a great tool!

Actual result

Flask graphite (source) normalizes slashes / in endpoint paths by converting them to pipe character |. For example:

stats.users.localhost.flask-graphite.data|users.size

Although such metrics are supported by Carbon, you have to manually escape the metric name in order to render the graph: stats.users.localhost.flask-graphite.data\|users.size. Unfortunately, this is not done by Graphite web, and we end up with "No data" screen for the metrics corresponding to hierarchical endpoint paths.

Note that Pipe character is considered a special symbol by Graphite. So this behaviour may be intentional.

&target=asPercent(Server{1,2}.memory.used,Server{1,3}.memory.total,0)|exclude("MISSING")

Expected result

Flask-graphite should generate metric names supported by Graphite web.

Environment

Graphite – 1.1.5 Flask-graphite – 0.6.0

ramnes commented 5 years ago

Hey @shapiy, thanks for raising this issue!

I only have older versions of Graphite currently deployed to play with, so I can't tell for sure, but I guess it's due to 1.1.1 implementing that piping syntax. If you have flexibility on your side, that would be awesome that you'd try older versions to make sure of which Graphite version introduces that problem.

Anyway, what would you think that we:

shapiy commented 5 years ago

@ramnes I don't have a working customizable environment, but I am planning to run some experiments this week!

Thanks for asking for my opinion.

Not sure offhand what is the best candidate for the default path separator. Note that unilaterally changing the separator may corrupt the stats (in a way). Users will start to see duplicate metrics: the metrics with the pipe character and the new ones. Grafana dashboards may also rely on the pipe character as the separator.

Making the separator configurable is a good option I think, because it provides additional control over migration.

shapiy commented 5 years ago

Our current way to fix it, though this is not the best workaround, is to patch GraphiteStructuredFormatter, which is the default formatter of graphitesend.

GraphiteStructuredFormatter.cleaning_replacement_list.append(('|', '_'))

As you can see, we substitute pipes | with underscores _ (I believe, this is not something suitable for everyone).

P.S. flask-graphite does a good job on passing configuration options to graphitesend, but it is not possible to customize the formatter. Should we optionally consider it as a potential improvement?

ramnes commented 5 years ago

It seems to me that it is a good workaround, actually! I feel we should use formatters and remove that manual conversion from here.

P.S. flask-graphite does a good job on passing configuration options to graphitesend, but it is not possible to customize the formatter. Should we optionally consider it as a potential improvement?

The great thing is that it will work out of the box when we'll rebase on graphitesender, since it allows a formatter to be given in the main class constructor. Feel free to open a pull request if you want to make this happen!

shapiy commented 5 years ago

+1 I will try to

shapiy commented 5 years ago

The corresponding graphite web issue: https://github.com/graphite-project/graphite-web/issues/2400