feat(statsd) new metrics and more flexible configuration #2400

shashiranjan84 · 2017-04-17T19:07:05Z

Summary

Now each metric supports configurable stat type,
sample rate and customer identifier if applicable.
Custom prefix for stats's name
New metrics upstream_latency, kong_latency and status_count_per_user.

Full changelog

Schema updated to support new metrics
Schema updated to support stat_type, sample_rate and consumer_identifier
Added new schema checks and test

Issues resolved

FIX #1533

NOTE: depends on the PR on #2367

p0pr0ck5 · 2017-04-17T22:38:23Z

Failed tests seem related to these changes?

shashiranjan84 · 2017-04-17T22:46:37Z

@p0pr0ck5 test will fail because this PR depends on PR on #2367. So basically next needs to be synced with master

Tieske · 2017-05-01T15:23:25Z

kong/plugins/statsd/handler.lua

+local function allow_user_metric(message, identifier)
+  if message.consumer ~= nil and
+          message.consumer[identifier] ~= nil then
+    return true, message.consumer[identifier]


do we need the nil comparisons? can either of those values ever be false?

eg.

if message.consumer and message.consumer[identifier] then return true, message.consumer[identifier]

or even

local empty = {} local function allow_user_metric(message, identifier) local user_metric = (message.consumer or empty)[identifier] return user_metric ~= nil, user_metric end

Its just for readability, I have been told before to keep it explicit to make it more readable.

Tieske · 2017-05-01T15:27:23Z

kong/plugins/statsd/handler.lua

+      local stat_name = stat_name[metric_config.name]
+      local stat_value = stat_value[metric_config.name]
+      local gauge = gauges[metric_config.stat_type]
+      if stat_name ~= nil and gauge ~= nil and stat_value ~= nil then


if stat_name and gauge and stat_value then

same reason.

Tieske · 2017-05-01T15:27:45Z

kong/plugins/statsd/handler.lua

+      end
+    else
+      local gauge = gauges[metric_config.name]
+      if gauge ~= nil then


if gauge then

Tieske · 2017-05-01T16:00:00Z

kong/plugins/statsd/migrations/cassandra.lua

+
+      for _, plugin in ipairs(plugins) do
+        local new_metrics = {}
+        if plugin.config.metrics ~=nil then


~=nil is missing a space, and do we need the nil?

Tieske · 2017-05-01T16:01:43Z

kong/plugins/statsd/migrations/postgres.lua

+      end
+    end
+  }
+}


Why is the logic for this migration different from the Cassandra one? the migrations runs through the dao so shouldn't it be identical?

End result is still same, its just adding a new row and deleting the old one.

but then for maintainability, let's use the same logic everywhere

I took hint from another migrations from mashape-analytics.

Tieske · 2017-05-01T16:10:43Z

kong/plugins/statsd/schema.lua

+    end
+  end
+  return false
+end


Don't use functions like this. Make a hash table and look it up, eg.

local consumer_identifiers = { ["consumer_id"] = true, ["custom_id"] = true, ["username"] = true, } if consumer_identifiers[somevalue] then ...

Yes I ll update the logic, though it will hardly make any difference as consumer_identifiers is a fixed length table.

Tieske · 2017-05-01T16:10:50Z

kong/plugins/statsd/schema.lua

+      return false, "unique_users metric only works with stat_type 'set'"
+    end
+    if (entry.stat_type == "counter" or entry.stat_type == "gauge") and ((entry.sample_rate == nil) or (entry.sample_rate ~= nil and type(entry.sample_rate) ~= "number") or (entry.sample_rate ~= nil and entry.sample_rate < 1)) then
+      return false, "sample rate must be defined for counters and gauges."


can you break those lines up for readability?

p0pr0ck5 · 2017-06-09T19:24:56Z

kong/plugins/statsd/migrations/postgres.lua

+          config  = {
+            host    = statsd.config.host,
+            port    = statsd.config.port,
+            metrics = default_metrics,


so this replaces the existing metrics with default values, is there any way to carry over plugin configs as part of this migration? or is that impossible?

p0pr0ck5 · 2017-06-09T19:25:30Z

kong/plugins/statsd/migrations/postgres.lua

+
+        -- Delete the old one to avoid conflicts when inserting the new one
+        local _, err = dao.plugins:delete(statsd)
+        if err then return err end


prefer avoiding one-line conditionals, we will be removing these in #2586

p0pr0ck5 · 2017-06-21T22:48:13Z

Looks like there's a merge conflict here.

Also, based on reading the migrations, it looks like we eliminate all old configurations. Is this desirable (or am I misreading)?

shashiranjan84 · 2017-06-21T23:05:13Z

@p0pr0ck5 Conflict resolved.

Migration removing the old config and adding it back with new changes.

Tieske

@shashiranjan84 sorry for some stuff that I reviewed before, but it's been a while... (I personally hate it when new stuff comes up in the second review)

Tieske · 2017-06-22T17:40:51Z

kong/plugins/statsd/handler.lua

+local ngx_timer_at = ngx.timer.at
+local string_gsub  = string.gsub
+local pairs        = pairs
+local format       = string.format


nitpick: for consistency use string_format = string.format

Tieske · 2017-06-22T17:49:58Z

kong/plugins/statsd/schema.lua

+  ["request_per_user"]      = true,
+  ["upstream_latency"]      = true,
+  ["kong_latency"]          = true,
+  ["status_count_per_user"] = true,


fyi: when the key is a valid Lua identifier name, you can omit the quotes and brackets:

local metrics = { request_count = true, latency = true,

would do the job
(no need to change)

Tieske · 2017-06-22T17:51:38Z

kong/plugins/statsd/schema.lua

+        and not entry.consumer_identifier then
+
+      return false, "consumer_identifier must be defined for metric "
+             .. entry.name


style, put the .. at the end of the previous line

Tieske · 2017-06-22T17:51:49Z

kong/plugins/statsd/schema.lua

+       and not consumer_identifiers[entry.consumer_identifier] then
+
+        return false, "invalid consumer_identifier for metric '" .. entry.name
+               .. "'. Choices are consumer_id, custom_id, and username"


style: same

Tieske · 2017-06-22T17:52:29Z

kong/plugins/statsd/schema.lua

+    prefix =
+    {
+      type     = "string",
+      default  = "kong"


style: trailing comma

Tieske · 2017-06-22T17:56:11Z

kong/plugins/statsd/statsd_logger.lua

    return
  end
 end

+


closing a socket always succeeds, no need for error checking here

function statsd_mt:close_socket() return self.socket:close() end

Mmmm, no, not true:

https://github.com/openresty/lua-nginx-module#udpsockclose

hmm right, I stand corrected 😄

Tieske · 2017-06-22T17:59:35Z

spec/03-plugins/06-statsd/02-schema_spec.lua

+    local metrics_input = {}
+    local ok, err = validate_entity({ metrics = metrics_input}, statsd_schema)
+    assert.is_nil(err)
+    assert.True(ok)


style: iirc the preferred style is to use assert.is_true as opposed to the capitalized version.

shashiranjan84 · 2017-06-22T19:10:32Z

@p0pr0ck5 @Tieske done.

p0pr0ck5

A few minor things to clean up, nothing that needs another round of review.

p0pr0ck5 · 2017-06-22T22:27:43Z

kong/plugins/statsd/handler.lua

+local string_gsub  = string.gsub
+local pairs        = pairs
+local string_format = string.format
+local NGX_ERR      = ngx.ERR


can we align these whitespaces

p0pr0ck5 · 2017-06-22T22:28:20Z

kong/plugins/statsd/migrations/cassandra.lua

+return {
+  {
+    name = "2017-06-09-160000_statsd_schema_changes",
+    up = function(_, _, dao)


can we add an empty 'down' function to these

- Now each metric supports configurable stat type, sample rate and customer identifier if applicable. - New metrics `upstream_latency`, `kong_latency` and `status_count_per_user`. - Custom prefix for stat's name - Code style refomatted

Applied all his advised chances with few exception

Tieske · 2017-06-23T08:45:55Z

yay, finally done!

p0pr0ck5 added the pr/status/please review label Apr 17, 2017

This was referenced Apr 18, 2017

docs(datadog) new metrics, tags and metrics flexible config Kong/docs.konghq.com#335

Closed

docs(statsd) new metrics and flexible configurability Kong/docs.konghq.com#334

Closed

shashiranjan84 added pr/status/do not merge (to discuss) and removed pr/status/please review labels Apr 18, 2017

shashiranjan84 force-pushed the feat/configurable-statsd branch from 5a43c1b to 0a101ef Compare April 18, 2017 00:42

shashiranjan84 added pr/status/please review and removed pr/status/do not merge (to discuss) labels Apr 18, 2017

Tieske requested changes May 1, 2017

View reviewed changes

Tieske added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 1, 2017

shashiranjan84 force-pushed the feat/configurable-statsd branch from 0a101ef to 2429c8d Compare May 1, 2017 17:27

shashiranjan84 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 1, 2017

thibaultcha added this to the 0.11.0 milestone May 19, 2017

thibaultcha assigned shashiranjan84 May 30, 2017

shashiranjan84 force-pushed the feat/configurable-statsd branch from 2429c8d to dc19e59 Compare June 1, 2017 23:15

shashiranjan84 added pr/status/do not merge (to discuss) and removed pr/status/please review labels Jun 1, 2017

shashiranjan84 force-pushed the feat/configurable-statsd branch from dc19e59 to 9733c5b Compare June 5, 2017 22:08

shashiranjan84 changed the title ~~feat(plugins/statsd) new metrics and more flexible configuration~~ feat(statsd) new metrics and more flexible configuration Jun 5, 2017

shashiranjan84 force-pushed the feat/configurable-statsd branch 2 times, most recently from 784ecb0 to dd461f6 Compare June 6, 2017 17:43

shashiranjan84 mentioned this pull request Jun 6, 2017

feat(statsd): Allow user defined metrics prefix #2551

Closed

shashiranjan84 added the pr/status/please review label Jun 6, 2017

shashiranjan84 force-pushed the feat/configurable-statsd branch 2 times, most recently from fed5858 to df28326 Compare June 6, 2017 21:56

shashiranjan84 removed the pr/status/do not merge (to discuss) label Jun 6, 2017

p0pr0ck5 reviewed Jun 9, 2017

View reviewed changes

shashiranjan84 force-pushed the feat/configurable-statsd branch 3 times, most recently from 1e2ae3d to 20205e8 Compare June 13, 2017 01:37

p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jun 21, 2017

p0pr0ck5 mentioned this pull request Jun 21, 2017

feat(datadog) new metrics and more flexible configuration #2394

Merged

shashiranjan84 force-pushed the feat/configurable-statsd branch from 20205e8 to c435460 Compare June 21, 2017 23:02

shashiranjan84 force-pushed the feat/configurable-statsd branch from c435460 to 5d0d5be Compare June 21, 2017 23:14

shashiranjan84 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jun 21, 2017

Tieske previously requested changes Jun 22, 2017

View reviewed changes

shashiranjan84 force-pushed the feat/configurable-statsd branch 2 times, most recently from 7854286 to f9b7234 Compare June 22, 2017 19:07

p0pr0ck5 previously approved these changes Jun 22, 2017

View reviewed changes

shashiranjan84 dismissed p0pr0ck5’s stale review via b621c7f June 22, 2017 22:51

shashiranjan84 force-pushed the feat/configurable-statsd branch from f9b7234 to b621c7f Compare June 22, 2017 22:51

p0pr0ck5 approved these changes Jun 22, 2017

View reviewed changes

shashiranjan84 added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/please review labels Jun 22, 2017

shashiranjan84 merged commit e635a34 into next Jun 22, 2017

Tieske deleted the feat/configurable-statsd branch June 23, 2017 08:45

p0pr0ck5 mentioned this pull request Aug 7, 2017

statsd_logger instance counter method throws no method error #2782

Closed

kikito mentioned this pull request Nov 17, 2017

Custom_ID instead of consumer_id on statsd plugin #1533

Closed

feat(statsd) new metrics and more flexible configuration #2400

feat(statsd) new metrics and more flexible configuration #2400

Conversation

shashiranjan84 commented Apr 17, 2017 • edited Loading

Summary

Full changelog

Issues resolved

p0pr0ck5 commented Apr 17, 2017

shashiranjan84 commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0pr0ck5 commented Jun 21, 2017

shashiranjan84 commented Jun 21, 2017

Tieske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 commented Jun 22, 2017

p0pr0ck5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tieske commented Jun 23, 2017

shashiranjan84 commented Apr 17, 2017 •

edited

Loading