fix(admin-api/status) removed counters and added simple DB ping #2554

shashiranjan84 · 2017-05-24T21:23:50Z

Summary

In favor to make /status admin API fast and constant time execution(short of), counters are removed and DB reachability status added.

Full changelog

Added reachability check to DB is done by firing a light query
to check if keyspace/database exists in DB.
Removed counters from /status api response and reachable status added.
Updated related test.

Note: Existing version of DataDog agent will not work with above fix.
A separate PR would be sent to DD repo to fix that too.

subnetmarco · 2017-05-24T21:32:28Z

Shouldn't this PR also update the plugins that are currently leveraging on the counters?

p0pr0ck5

minor style comments, but lgtm otherwise.

p0pr0ck5 · 2017-05-24T21:25:46Z

kong/dao/db/cassandra.lua

-    -- return time in UNIT millisecond, and PRECISION millisecond 
-    return math.floor(timestamp.get_utc_ms()) 
+    -- return time in UNIT millisecond, and PRECISION millisecond
+    return math.floor(timestamp.get_utc_ms())


can we avoid these whitespace cleanups. glad we're cleaning up, but it dirties the commit history. we can have a separate style clean commit later.

Unintentional fix done by IDE. I'll take care of this next time.

p0pr0ck5 · 2017-05-24T21:26:54Z

kong/dao/db/postgres.lua

@@ -497,6 +497,7 @@ function _M:current_migrations()
  end
 end

+


another unnecessary change?

p0pr0ck5 · 2017-05-24T21:30:53Z

kong/api/routes/kong.lua

+      status_response.database.reachable, err = dao.db:is_reachable()
+
+      if err ~= nil then
+        ngx.log(ngx.ERR, err)


should we report the error, or a summary of the error, as part of the response? my gut feeling is 'no', just bringing this up as a discussion point

I had this question, but I guess its okay to just return status.

We need to at least give context to this error:

ngx.log(ngx.ERR, "failed to reach database as part of ", "/status endpoint: ", err)

p0pr0ck5 · 2017-05-24T21:32:50Z

kong/dao/db/cassandra.lua

+    prepared = false
+  }, nil, true)
+
+  if not rows then return false, err end


for style, i think we avoid one-line conditional blocks?

Not sure, personally I would prefer indented conditional block, but I Kong is full of one-liner conditional statements.

Those are legacy, please use indented conditional branches indeed.

Additionally, and by convention, we return nil, err on error, and not false. This is also valid later down this function.

shashiranjan84 · 2017-05-24T21:34:54Z

@thefosk no, counter method is still there and being referenced at many places. So it would need separate card/focus.

subnetmarco · 2017-05-24T21:45:56Z

Don't those agents rely on the counters returned by the /status endpoint?

shashiranjan84 · 2017-05-24T21:48:23Z

@thefosk sorry, what agent you talking about? Only agent I am aware of is DataDog, which I already put in note.

thibaultcha

Great, the overall logic LGTM 👍 But this needs some more polishing.

Thank you!

thibaultcha · 2017-05-25T05:18:32Z

kong/api/routes/kong.lua

+      status_response.database.reachable, err = dao.db:is_reachable()
+
+      if err ~= nil then
+        ngx.log(ngx.ERR, err)


We need to at least give context to this error:

ngx.log(ngx.ERR, "failed to reach database as part of ", "/status endpoint: ", err)

thibaultcha · 2017-05-25T05:19:19Z

kong/dao/db/cassandra.lua

+    prepared = false
+  }, nil, true)
+
+  if not rows then return false, err end


Those are legacy, please use indented conditional branches indeed.

thibaultcha · 2017-05-25T05:20:50Z

kong/dao/db/cassandra.lua

+    prepared = false
+  }, nil, true)
+
+  if not rows then return false, err end


Additionally, and by convention, we return nil, err on error, and not false. This is also valid later down this function.

thibaultcha · 2017-05-25T05:21:09Z

kong/dao/db/postgres.lua

@@ -516,4 +516,21 @@ function _M:record_migration(id, name)
  return true
 end

+function _M:is_reachable()
+


this blank line is undesired

thibaultcha · 2017-05-25T05:22:34Z

kong/dao/db/postgres.lua

+  local query = fmt(
+    [[
+      SELECT * FROM pg_stat_database where datname='%s'
+    ]], self.query_options.database)


I'm not sure we need the multi-line string here:

local query = fmt("SELECT * FROM pg_stat_database where datname='%s'", self.query_options.database)

thibaultcha · 2017-05-25T05:22:45Z

kong/dao/db/postgres.lua

+    ]], self.query_options.database)
+
+  local rows, err = self:query(query)
+  if not rows then return false, err end


ditto: indented block vs single line and return failure value (same below)

thibaultcha · 2017-05-25T05:26:11Z

kong/api/routes/kong.lua

-        status_response.database[k] = count
+      -- ping DB
+      local err
+      status_response.database.reachable, err = dao.db:is_reachable()


Due to the below changes with the return value (and because this reachable() function might be used elsewhere and should not be tied to this endpoint for the sake of modularisation), this could become:

local ok, err = dao.db:reachable() if not ok then ngx.log(ngx.ERR, "msg...", err) else status_response.database.reachable = true end

thibaultcha · 2017-05-25T05:26:41Z

kong/dao/db/cassandra.lua

@@ -660,4 +660,36 @@ function _M:record_migration(id, name)
  return true
 end

+function _M:is_reachable()


We prefer less cumber-stone names, here, reachable() should better suited.

- removed counter for each Kong tables being returned as part of `/status` api response. - reachablity check to DB is done by firing a light query to check if keyspace/database exists. Note: Existing version of DataDog agent will not work with above fix. A separate PR would be sent to DD repo to fix that too.

shashiranjan84 · 2017-05-25T16:49:15Z

Updated as advised.

thibaultcha · 2017-05-25T17:57:50Z

LGTM, I'll merge manually with some minor leftover edits

shashiranjan84 · 2017-05-25T18:01:01Z

Thanks @thibaultcha.

thibaultcha · 2017-05-26T00:07:54Z

On second thought, I do think the logic here is not totally aligned with what has been discussed... See #2567.

From #2554

thibaultcha · 2017-05-30T18:34:36Z

Done with #2567

shashiranjan84 added the pr/status/please review label May 24, 2017

p0pr0ck5 reviewed May 24, 2017

View reviewed changes

shashiranjan84 force-pushed the fix/status branch from 93cb2ef to d69f277 Compare May 24, 2017 21:42

thibaultcha requested changes May 25, 2017

View reviewed changes

shashiranjan84 force-pushed the fix/status branch from d69f277 to 7ec50c3 Compare May 25, 2017 16:34

shashiranjan84 force-pushed the fix/status branch from 7ec50c3 to 356345d Compare May 25, 2017 16:46

thibaultcha mentioned this pull request May 26, 2017

feat(admin) remove DB counters in favor of a connection test #2567

Merged

thibaultcha added a commit that referenced this pull request May 26, 2017

feat(admin) remove DB counters in favor of a connection test

a564e48

From #2554

thibaultcha added a commit that referenced this pull request May 30, 2017

feat(admin) remove DB counters in favor of a connection test

7a8a42b

From #2554

thibaultcha closed this May 30, 2017

thibaultcha deleted the fix/status branch May 30, 2017 18:34

subnetmarco mentioned this pull request Aug 3, 2017

Configurable Proxy Timeout in Kong Dashboard PGBI/kong-dashboard#118

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(admin-api/status) removed counters and added simple DB ping #2554

fix(admin-api/status) removed counters and added simple DB ping #2554

shashiranjan84 commented May 24, 2017 •

edited

Loading

subnetmarco commented May 24, 2017

p0pr0ck5 left a comment

p0pr0ck5 May 24, 2017

shashiranjan84 May 24, 2017

p0pr0ck5 May 24, 2017

shashiranjan84 May 24, 2017

p0pr0ck5 May 24, 2017

shashiranjan84 May 24, 2017 •

edited

Loading

thibaultcha May 25, 2017

p0pr0ck5 May 24, 2017

shashiranjan84 May 24, 2017 •

edited

Loading

thibaultcha May 25, 2017

thibaultcha May 25, 2017

shashiranjan84 commented May 24, 2017

subnetmarco commented May 24, 2017

shashiranjan84 commented May 24, 2017

thibaultcha left a comment

thibaultcha May 25, 2017

thibaultcha May 25, 2017

thibaultcha May 25, 2017

thibaultcha May 25, 2017

thibaultcha May 25, 2017

thibaultcha May 25, 2017

thibaultcha May 25, 2017

thibaultcha May 25, 2017

shashiranjan84 commented May 25, 2017

thibaultcha commented May 25, 2017

shashiranjan84 commented May 25, 2017

thibaultcha commented May 26, 2017

thibaultcha commented May 30, 2017

fix(admin-api/status) removed counters and added simple DB ping #2554

fix(admin-api/status) removed counters and added simple DB ping #2554

Conversation

shashiranjan84 commented May 24, 2017 • edited Loading

Summary

Full changelog

subnetmarco commented May 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashiranjan84 commented May 24, 2017

subnetmarco commented May 24, 2017

shashiranjan84 commented May 24, 2017

thibaultcha 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

shashiranjan84 commented May 25, 2017

thibaultcha commented May 25, 2017

shashiranjan84 commented May 25, 2017

thibaultcha commented May 26, 2017

thibaultcha commented May 30, 2017

shashiranjan84 commented May 24, 2017 •

edited

Loading

shashiranjan84 May 24, 2017 •

edited

Loading

shashiranjan84 May 24, 2017 •

edited

Loading