Rethink :series_name_for_x configuration

[dmke]

I'm contemplating to reduce the number of config options for the series this gem writes creates:

https://github.com/influxdata/influxdb-rails/blob/cbfd10b6a9611ae52b202896c98b6ff29e3d0a29/lib/influxdb/rails/configuration.rb#L19-L23

with these default values:

https://github.com/influxdata/influxdb-rails/blob/cbfd10b6a9611ae52b202896c98b6ff29e3d0a29/lib/influxdb/rails/configuration.rb#L63-L67

Reasons

1. The nomenclature is misleading, it stems from some time ago, when "series name" was the right vocabulary. These attributes actually mean "measurement name". A "series" in InfluxDB 0.9+ means values for a given measurement.
2. Aggregating across multiple measurements is hard (if not impossible). The current setup needs to send five queries to InfluxDB to get the full picture. The PR Implement render_template & render_partial subscriptions #53 adds three more, and Implement SQL Subscriber #55 will add another.

Proposed change

Let there be only one measurement configurable, say Configuration#measurement_name with default value "rails". What was previously written to different measurements, should now get an additional tag to differentiate the source.

Exemplary change for InfluxDB::Rails::Instrumentation:

@@ def benchmark_for_instrumentation @@
 InfluxDB::Rails.client.write_point \
-  c.series_name_for_instrumentation,
+  c.measurement_name,
   values: {
     value: ((Time.now - start) * 1000).ceil,
   },
   tags:   configuration.tags_middleware.call(
+    series: "instrumentation",
     method: "#{controller_name}##{action_name}",
     server: Socket.gethostname
   )

N.B. Not having a dot in the measurement name also makes writing the queries easier, as it does not need to be escaped, because the from clause may have three incarnations:

1. FROM <database_name>.<retention_policy_name>.<measurement_name> – full qualified
2. FROM <database_name>..<measurement_name> – using default retention policy
3. FROM <measurement_name> – using current database and default retention policy

without paying attention, this does raise an error ("retention policy not found"):

select * from rails.db

By eliminating the dot in the name, it becomes easier:

select * from "rails.db" where ...            -- before: needs escaping
select * from rails where series = 'db' ...   -- after: less escaping

Impact on existing users

Existing data could be migrated, and we should support users in doing so. I've already written a short script to migrate the exception measurement (#43), which could be used as a guide.

Existing tooling will break, but changes should be straightforward. For Grafana users, we could create an "official" dashboard to ease the transition. This is a stretch goal though, so it should go into a separate issue.

Tasks

• modify README.md
• change InfluxDB::Rails::Configuration
• update subscriber classes
• create upgrade guide for pre-1.0 users

[ChrisBr]

For Grafana users, we could create an "official" dashboard to ease the transition. This is a stretch goal though, so it should go into a separate issue.

This would be a HUGE benefit. We still struggle to build a meaningful dashboard 😢 Our public openSUSE Grafana dashboard is located here if you want to have a look:

https://metrics.opensuse.org/d/qjkd7uBmz/rails-metrics?orgId=1&from=now%2Fd&to=now%2Fd

I would volunteer to helper here but definitely need some guidance as I'm still new to InfluxDB and Grafana unfortunately.

[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.

[ChrisBr]

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.

[dmke]

Btw. let me move this conversation to another issue. → #60