search

fluent / fluent-plugin-prometheus

A fluent plugin that collects metrics and exposes for Prometheus.
Apache License 2.0
258 stars 79 forks source link

implement Flunet::Filter#filter instead of Flunet::Filter#filter_stream #124

Closed ganmacs closed 4 years ago

ganmacs commented 5 years ago

Default implementation of Flunet::Filter#filter_stream has rescue statement by default. so if received record is invalid, it would work without any error. and It can be faster by using fileter optimization feature (https://docs.fluentd.org/filter#filter-chain-optimization)

repeatedly commented 5 years ago

Using filter seems good approach but I think this implementation is not good for memory because instrument creates placeholder value internally.

https://github.com/fluent/fluent-plugin-prometheus/blob/aaef57a777a3732438652d5320dcd29bfc04299a/lib/fluent/plugin/prometheus.rb#L95

So to avoid such problem, light-weight instrument implementation is needed. From my view, implementing own expander is better: https://github.com/fluent/fluent-plugin-prometheus/blob/aaef57a777a3732438652d5320dcd29bfc04299a/lib/fluent/plugin/prometheus.rb#L86 Current implementation depends on record_transformer. This isn't robust because record_transformer's internal class may be changed.

ganmacs commented 5 years ago

Using filter seems good approach but I think this implementation is not good for memory because instrument creates placeholder value internally.

Sorry. I didn't get why it's not good for memory. placeholder_value is a Hash object, right? Is creating a this hash each record problem?

Current implementation depends on record_transformer. This isn't robust because record_transformer's internal class may be changed.

Right. but I think it should be done in another PR to minimize the change.

ganmacs commented 4 years ago

@repeatedly ping

repeatedly commented 4 years ago

Is creating a this hash each record problem?

I'm not sure the impact. Old implementation creates only 1 placholder hash per event steam: https://github.com/fluent/fluent-plugin-prometheus/blob/aaef57a777a3732438652d5320dcd29bfc04299a/lib/fluent/plugin/prometheus.rb#L96 But new implementation creates placeholder hash per record. So if event steam contains 1000 events, new implementation creates 1000 records even if tag is same.

Right. but I think it should be done in another PR to minimize the change.

Yeah, I think so.

ganmacs commented 4 years ago

cached placeholder_value by tag for safety. https://github.com/fluent/fluent-plugin-prometheus/pull/124/commits/f58e9427abb4e0d62654c7ec9eb56259f3318943

ganmacs commented 4 years ago

I created a dedicated method for handling signle record and reverted all changes about #instrument

https://github.com/fluent/fluent-plugin-prometheus/pull/124/commits/31cf62df55f6013e741468bead3822d930e9ae36

repeatedly commented 4 years ago

@kazegusuri How about this patch? If you are ok, we want to release new version with this change.

kazegusuri commented 4 years ago

It seems okay. Just a question and my curious. Is this simple for performance improvement?

ganmacs commented 4 years ago

Is this simple for performance improvement?

Yes. In addition, if record is nil for some reasons, utilizing fluentd core's rescue logic.