Generalize SimpleSubscriber

[dmke]

From @ChrisBr:

I would also like to refactor the SimpleSubscriber to be useful out of the box and users can just subscribe to any ActiveSupport hook like this:

series_name = 'series_name'
tags = [:foo, :bar]
normalizer = Proc.new { }
subscriber = InfluxDB::Rails::Subscriber.new('series_name'
ActiveSupport::Notifications.subscribe('the_notification', tags, normalizer)

The tags would be an array of the tags which should be extracted from the notification data. The normalizer is a proc which gets called with the data and optinally transforms it.

Not sure yet how the data will be structured in the influxdb though. What do you think?

[dmke]

users can just subscribe to any ActiveSupport hook

That's a useful addition.

Not sure yet how the data will be structured in the influxdb though.

At its core, it's "just" a key-value store. Some keys are special, in that their associated values con only be strings, but are indexed (tags), one key is used for absolute ordering (time), and the other keys may hold arbitrary values (fields).

The challenge, if we open that for general use, is to communicate the distinction between tags and fields. Tags should, generally speaking, only hold data with low cardinality (i.e. the distinct set of values small). You've seen in #50/#54 that unbound values can cause problems. Subscriber implementators should be made aware of this, at least with an API that is hard to misuse.

[dmke]

users can just subscribe to any ActiveSupport hook

That's a useful addition.

Not sure yet how the data will be structured in the influxdb though.

At its core, it's "just" a key-value store. Some keys are special, in that their associated values con only be strings, but are indexed (tags), one key is used for absolute ordering (time), and the other keys may hold arbitrary values (fields).

The challenge, if we open that for general use, is to communicate the distinction between tags and fields. Tags should, generally speaking, only hold data with low cardinality (i.e. the distinct set of values small). You've seen in #50/#54 that unbound values can cause problems. Subscriber implementators should be made aware of this, at least with an API that is hard to misuse.

Subscriber implementators should be made aware of this, at least with an API that is hard to misuse.

Yep, right. What about adding a blacklist of keys from Active Support instrumentation and write an error message to the logs? The user could still misuse it but at least they get an notification. The blacklist would e.g. contain [:identifier, :id, :key, :to, :from, :cc, :bcc, :date ...]. I don't think that the black list would be extensive. Maybe we could even be more restrictive and just don't send those tags to InfluxDB and write in the documentation that we remove these tags because of their cardinality.

[dmke]

write an error message to the logs? [...] Maybe we could even be more restrictive and just don't send those tags to InfluxDB and write in the documentation that we remove these tags because of their cardinality.

Unless it's an exception, I doubt most users will ever look into the production.log (then again, the typical use case for this gem isn't the 15-minute blog app). Also, silently dropping the data points would not be what the user expects. If they normalize some value, they'll wonder why that value doesn't show up in the server measurement, only to discover after hours of debugging, that the key is blacklisted for some reason.

What about adding a blacklist of keys

That might work, but it creates a maintenance burden: whenever the payload keys are changed upstream, we propably need to modify this gem as well (I don't have the resources (i.e. time) to do that).

I was more thinking of something like

InfluxDB::Rails.subscribe(
  series:         "series_name",
  notification:   "the_notification"
  payload_tags:   %i[t1 t2 t3],
  payload_fields: %i[f1 f2]
) do |tags, fields|
  # optional normalizing
end

and have a bit of documentation explaining the payload_* keys.

[hennevogel]

This is handled a bit differently in #107 now