fluent / fluent-plugin-prometheus

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

fix: allow for lookup by symbol as well as string #186

Closed aauren closed 3 years ago

aauren commented 3 years ago

I've noticed in my testing of this plugin, that sometimes I'm not able to instrument my metrics properly. Even though I know that I'm sending correct values, the prometheus plugin doesn't seem to ever pick up the changes.

After doing a fair amount of debugging, I've found that sometimes fluentd will pass the Hash's of my messages to the prometheus plugin using symbols to key the hash instead of strings. As Ruby treats symbols as different than strings, this means that the prometheus plugin isn't able to parse the message.

Here is an example of one such message...

When I output it from my plugin it is sent as:

{"name":"foo-6b87bf6b57-46czk","count":6472}

If I insert debug logging into the metric instrumentation method of this plugin, here is what it receives from fluentd:

{:name=>"foo-6b87bf6b57-46czk", :count=>6472}

This fails, when it tries to use the key, I provide in my config:

    <metric>
      name fluentd_custom_lines_by_podname
      type counter
      desc The total number of lines received by podname
      key count
    </metric>

Because it essentially tries to use record["count"] when it instead needs to be using record[:count]

This PR changes the default functionality for all instrumentations to fallback to using symbols if indexing by string fails to work properly.

aauren commented 3 years ago

I changed my approach here a bit after I realized that if the keys to the hash aren't strings it affects a lot of other methods, including the ability to call() the built-in fluentd record_accessor.rb helper which then uses string based keys to call dig().

So now instead of adding independent handling of the hash keys at certain steps, I just string-ify the all of the keys in the record before doing any processing.

aauren commented 3 years ago

@kazegusuri @repeatedly - Any chance that either one of you would be willing to take a look at this? Right now we maintain a patch that we apply with this change, but it would be nice to be able to get past this bug with an upstream solution.

cosmo0920 commented 3 years ago

I added symbolized keys case unit test. Yeah, this seems to be reasonable for me.

aauren commented 3 years ago

Thanks you!

And thanks for doing some extra cleanup work on my behalf to make it merge-able!