Implement render_template & render_partial subscriptions

[ChrisBr]

Initial proposal to implement subscriptions for 👀

• render_template.action_view
• render_partial.action_view

https://guides.rubyonrails.org/active_support_instrumentation.html#action-view

• Implemented ActionView::RenderSubscription class to handle the events and write to influxdb
• Extracted convert_timestamp method to a utils class

I would also propose to move InfluxDB::Rails.handle_action_controller_metrics and the lambda in Railties class to a dedicated class.

Thanks for providing feedback ❤️

[ChrisBr]

Ok, added now some unit and integration tests.

[dmke]

Sorry, I'm currently a bit busy. Give me a few days to get things in order.

At a first glance this looks promising. I'm not a big fan of Util classes, as they tend to end up cluttered with unrelated methods. For now, this is OK, but I'm tempted to move it toinfluxdb-ruby itself (as I can see the usefulness for other consumers).

I would also propose to move InfluxDB::Rails.handle_action_controller_metrics and the lambda in Railties class to a dedicated class.

If that clears things up, go ahead.

[ChrisBr]

Sorry, I'm currently a bit busy. Give me a few days to get things in order.

No worries, take your time ⌚

At a first glance this looks promising. I'm not a big fan of Util classes, as they tend to end up cluttered with unrelated methods. For now, this is OK, but I'm tempted to move it to influxdb-ruby itself (as I can see the usefulness for other consumers).

Right. Maybe we can move it to a TimeConversion module to limit the scope of the mixin for now?

If that clears things up, go ahead.

Ok, cool! I will try to move it to a dedicated class then.

[dmke]

FYI: I've relocated the convert_timestamp method to the influxdb gem (v0.6.2, InfluxDB.convert_timestamp), and bumped the dependency here.

[ChrisBr]

FYI: I've relocated the convert_timestamp method to the influxdb gem (v0.6.2, InfluxDB.convert_timestamp), and bumped the dependency here.

👍 cool, I will update tomorrow.

I added two more features:

• 6517dc3: Adding the location to the view renderer to track down in which controller action the view was renderer

• 1042b80: Tracking of ActiveRecord SQL, the SQL gets normalized before inserted as tags

Thanks

[dmke]

• 6517dc3: Adding the location to the view renderer to track down in which controller action the view was renderer

Neat :-)

• 1042b80: Tracking of ActiveRecord SQL, the SQL gets normalized before inserted as tags

After reverting payload[:path] (#54), I wonder if this wouldn't have the same effect... I would rather have the normalized query as field value, and use table name and operation as tags (i.e. select * from users where id = xxx would yield tags: { table_name: "users", op: "select" }, values: { query: "select * ..." }). What do you think?

[ChrisBr]

@dmke I rebased now on the tag middleware changes and also removed the timestamp conversion module.

After reverting payload[:path] (#54), I wonder if this wouldn't have the same effect... I would rather have the normalized query as field value, and use table name and operation as tags (i.e. select * from users where id = xxx would yield tags: { table_name: "users", op: "select" }, values: { query: "select * ..." }). What do you think?

I guess / hope not as I only track SELECT INSERT UPDATE DELETE and they get normalized from their parameters so I guess the number of different queries will be limited. But I can try it out for a few days and check what happens...

I just would like to do queries like:

SELECT value FROM rails.sql GROUP_BY sql;

to see the slowest queries. I'm not too familiar with InfluxDB yet but would that be possible with a field value or what would be the advantage of the field value except that it is not limited in the number of different values like a tag?).

I can also move this last commit to a dedicated PR so that we can get in the PR and continue discussion in the other PR?

[ChrisBr]

I would like to get something like this:

Skylight does something similar, they combine it with the partial and view results:

Would that be possible with a field value? Otherwise maybe we can normalize the queries even more? But I really see a huge advantage in measuring this data 🔍

[dmke]

I guess / hope not as I only track SELECT INSERT UPDATE DELETE and they get normalized from their parameters so I guess the number of different queries will be limited. But I can try it out for a few days and check what happens...

I'll sample a few queries from a few different Rails apps I've access to, and throw it against your normalizer. I suspect the interesting queries (those with sub-sejects, joins, and/or group-by clauses) are going to be boiled down too much. I fear that the simple regex is both too simple has a too large CPU footprint (I've seen hundrets of queries on a single page).

I just would like [...] to see the slowest queries. I'm not too familiar with InfluxDB yet but would that be possible with a field value or what would be the advantage of the field value except that it is not limited in the number of different values like a tag?).

Tags are automatically indexed and allow to be GROUP BY'ed on the server. Fields may hold arbitrary data, but you cannot use field keys in GROUP BY clauses. You may want to GROUP BY sql, if you need to sample the average (or other percentiles) for analysis.

Say you want to see the average query time in the last 24 hours in 1 hour slices, for each query. This is what you'd send to the server:

select mean(value) from "rails.sql" where time > now() - 1d group by time(1h), sql

If sql is a field, this would not be possible.

Skylight does something similar, they combine it with the partial and view results

That looks to me, as if the omit the conditions from the query (I see select from venues, but no select from venue where x in (y). This suggests two tags (table_name and op).

I can also move this last commit to a dedicated PR so that we can get in the PR and continue discussion in the other PR?

Yeah, let's do that for now.

[ChrisBr]

I can also move this last commit to a dedicated PR so that we can get in the PR and continue discussion in the other PR?

Yeah, let's do that for now.

Dropped and opened https://github.com/influxdata/influxdb-rails/pull/55

[dmke]

Apart from a few nitpicks, I like what I see.

[ChrisBr]

@dmke thanks for the review, implemented your suggestions.

[dmke]

I've changed a few bits (Rubocop was still producing output), but this is now merged.