Add pgbouncer input plugin #3918

nerzhul · 2018-03-21T22:19:40Z

This requires an update on jackc/pgx to support a specific feature (v3.1.0 was chosen)

This PR will fix #3253 and normalize the pgbouncer component properly

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

You will find a grafana output example below

nerzhul · 2018-03-21T23:07:21Z

I don't know if a such module can be merged for 1.6, this could be nice as we are finishing the qualification to use telegraf in our production farm to replace collectd, and currently we have some lacks in the postgresql queries & no pgbouncer support (i will provide postgresql queries later, for 1.7 i think)

danielnelson · 2018-03-21T23:12:24Z

Thanks for the pull request, I won't be able to review until 1.6 is out unfortunately.

nerzhul · 2018-03-22T09:22:44Z

No problem i can wait for 1.7 if needed, it's an internal qualification atm and we can keep collectd for a moment to finish our configurations

nerzhul · 2018-04-24T15:13:34Z

@danielnelson i hope you get time to review it i need to have it for 1.7 :) after merge i will improve the postgresql connector itself which has many missing data

nerzhul · 2018-05-11T14:06:23Z

@danielnelson i solved the conflict with master branch. I will continue working on postgres metrics as soon as you take this PR for an action or a merge on master to improve the poor pg support in telegraf

danielnelson · 2018-04-24T23:03:06Z

plugins/inputs/pgbouncer/README.md

+```
+[[inputs.pgbouncer]]
+  address = "postgres://telegraf@localhost/pgbouncer"
+```


Can you add details about the measurement/tags/fields you are collecting, check the EXAMPLE_README.md

danielnelson · 2018-04-24T23:05:17Z

plugins/inputs/pgbouncer/pgbouncer.go

+  ## Without the dbname parameter, the driver will default to a database
+  ## with the same name as the user. This dbname is just for instantiating a
+  ## connection with the server and doesn't restrict the databases we are trying
+  ## to grab metrics for.


I think this info about the dbname can be removed from the sample config, it will be enough to have it in the README.

danielnelson · 2018-04-25T01:01:18Z

plugins/inputs/pgbouncer/pgbouncer.go

+	return "Read metrics from one or many pgbouncer servers"
+}
+
+func (p *PgBouncer) IgnoredColumns() map[string]bool {


I don't think this function is used, instead the map is access directly. Probably just remove this function.

danielnelson · 2018-04-25T02:43:44Z

plugins/inputs/pgbouncer/pgbouncer.go

+		dbname.WriteString((*columnMap["database"]).(string))
+	} else {
+		dbname.WriteString("postgres")
+	}


I think this is too general purpose of a way to parse the stats. There are other columns we would probably want as tags such as pool_mode and user and I think it may be difficult to use a single function to parse all commands.

When new stats are added we will want to have a chance to decide if they are fields or tags, so I think they should be included manually, this does require a bit more upkeep but once a string is added as a field we can't switch it easily.

Unfortunately, I don't think we can specify the exact columns we are interested in, it would really nice to just be able to say select database,total_requests,... from stats;

I think we should return a map[string]interface{} from this function and have a function for each query to assign tags and call AddFields. For SHOW STATS the only tag would be database, and for SHOW POOLS you would have database, user, and pool_mode as tags.

Optionally, consider not even reporting avg_req | avg_recv | avg_sent | avg_query since these can all be calculated using the difference in totals.

danielnelson · 2018-05-14T22:08:33Z

plugins/inputs/pgbouncer/pgbouncer.go

+			fields[col] = *val
+		}
+	}
+	acc.AddFields("pgbouncer", fields, tags)


I think it will be best to have two measurements: pgbouncer and pgbouncer_pools since the values are unrelated.

nerzhul · 2018-05-14T22:42:40Z

Thanks for the review, i will fix the points soon

nerzhul · 2018-05-16T13:04:52Z

except the documentation points, all technical points are fixed @danielnelson . Can you review another time please ? i will complete the doc as soon as you are okay with the technical fixes

zaaraameera2014

Thanks u

nerzhul · 2018-05-16T14:31:46Z

@zaaraameera2014 i don't really understand why did you requested changes saying thanks you

danielnelson

Either whitelist the addition of fields or only add the fields if they are a numeric type; float or int. This will protect us against new string fields being added to pgbouncer that are either too long or would be better as tags.

danielnelson · 2018-05-16T20:59:06Z

plugins/inputs/pgbouncer/pgbouncer.go

+		}
+
+		tags["user"] = (*columnMap["user"]).(string)
+		tags["pool_mode"] = (*columnMap["pool_mode"]).(string)


Add checks around these in case they don't exist or are not strings.

freeseacher · 2018-07-17T17:47:11Z

hey anybody :) ? do smthing :)

nerzhul · 2018-07-17T18:56:45Z

Hey, thanks to remember me i need this, i need it myself at work but i'm on the metrology these days :)

Properly load it & ignore database column Add pgbouncer docker image for tests

nerzhul · 2018-07-17T21:07:10Z

@danielnelson i know i'm late but i fixed the issue, it's mergeable now, i hope for 1.8 ?

glinton · 2018-07-17T21:17:41Z

plugins/inputs/pgbouncer/pgbouncer.go

@@ -98,8 +98,15 @@ func (p *PgBouncer) Gather(acc telegraf.Accumulator) error {
 			return err
 		}

-		tags["user"] = (*columnMap["user"]).(string)
-		tags["pool_mode"] = (*columnMap["pool_mode"]).(string)
+		switch (*columnMap["user"]).(type) {


Instead of this switch, maybe do:

if s, ok := (*columnMap["pool_mode"]).(string); ok && s != "" { tags["user"] = s }

yes, you are right

glinton · 2018-07-24T16:00:34Z

I think once the last things daniel requested are complete, this will be good to go

nerzhul · 2018-07-24T16:03:24Z

@glinton i think i lost a commit when rebasing... yeah :(

glinton · 2018-07-24T16:08:32Z

ooh, not fun. You can merge master into your branch, rather than rebase, and push since we squash and merge, if that's more convenient for you

nerzhul · 2018-07-24T17:23:21Z

@glinton generally i never do that to have a proper history, but the problem was the force push on a non locally updated branch, which loose some commits

nerzhul · 2018-07-24T17:33:01Z

@glinton i think we are good. I found the original commit history after updating on my own GH repo (see nerzhul@c4c3838)
The ignored tags are handled. The other case is handled in this subcommit: nerzhul@0c41e8d

nerzhul · 2018-08-01T13:03:03Z

@glinton can we expect this on 1.8 ? the PR is waiting for a merge since a long time (i know i lost time too at a point), we really need this in production to replace our collectd

freeseacher · 2018-08-01T13:21:32Z

pretty crazy pgbouncer history with telegraf :)

glinton · 2018-08-01T17:33:48Z

I think this can be merged once @danielnelson verifies this review comment has been resolved. Thanks for the patience

nerzhul · 2018-08-02T06:26:53Z

Thanks for your time and the merge

nerzhul changed the title ~~Add pgbouncer support to telegraf~~ Add pgbouncer input plugin Mar 21, 2018

danielnelson added the new plugin label Mar 21, 2018

danielnelson reviewed May 14, 2018

View reviewed changes

nerzhul force-pushed the pgbouncer branch from c6f715b to 8ead219 Compare May 16, 2018 12:22

zaaraameera2014 suggested changes May 16, 2018

View reviewed changes

danielnelson reviewed May 16, 2018

View reviewed changes

nerzhul force-pushed the pgbouncer branch from c4c3838 to d0c242c Compare July 17, 2018 19:01

Add pgbouncer support to telegraf

3634ac7

Properly load it & ignore database column Add pgbouncer docker image for tests

nerzhul force-pushed the pgbouncer branch from d0c242c to 3634ac7 Compare July 17, 2018 21:01

Fix the variable type check in pgbouncer tags

15fda02

glinton reviewed Jul 17, 2018

View reviewed changes

Sexy tags type

fd3151a

PJMODOS mentioned this pull request Jul 27, 2018

Native Prometheus exporter support pgbouncer/pgbouncer#289

Closed

glinton added this to the 1.8.0 milestone Aug 1, 2018

glinton approved these changes Aug 1, 2018

View reviewed changes

danielnelson approved these changes Aug 1, 2018

View reviewed changes

glinton merged commit 429d141 into influxdata:master Aug 1, 2018

kramarz mentioned this pull request Aug 2, 2018

postgresql_extensible.query cannot connect to pgbouncer #3253

Closed

rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018

Add pgbouncer input plugin (influxdata#3918)

4f962dc

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019

Add pgbouncer input plugin (influxdata#3918)

fb56d7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pgbouncer input plugin #3918

Add pgbouncer input plugin #3918

nerzhul commented Mar 21, 2018 •

edited

Loading

nerzhul commented Mar 21, 2018 •

edited

Loading

danielnelson commented Mar 21, 2018

nerzhul commented Mar 22, 2018

nerzhul commented Apr 24, 2018

nerzhul commented May 11, 2018

danielnelson Apr 24, 2018

danielnelson Apr 24, 2018

nerzhul May 16, 2018

danielnelson Apr 25, 2018

nerzhul May 16, 2018

danielnelson Apr 25, 2018

nerzhul May 16, 2018

danielnelson May 14, 2018

nerzhul May 16, 2018

nerzhul commented May 14, 2018

nerzhul commented May 16, 2018

zaaraameera2014 left a comment

nerzhul commented May 16, 2018

danielnelson left a comment

danielnelson May 16, 2018

freeseacher commented Jul 17, 2018

nerzhul commented Jul 17, 2018

nerzhul commented Jul 17, 2018

glinton Jul 17, 2018

nerzhul Jul 17, 2018

glinton commented Jul 24, 2018

nerzhul commented Jul 24, 2018

glinton commented Jul 24, 2018

nerzhul commented Jul 24, 2018

nerzhul commented Jul 24, 2018

nerzhul commented Aug 1, 2018

freeseacher commented Aug 1, 2018

glinton commented Aug 1, 2018

nerzhul commented Aug 2, 2018

Add pgbouncer input plugin #3918

Add pgbouncer input plugin #3918

Conversation

nerzhul commented Mar 21, 2018 • edited Loading

Required for all PRs:

nerzhul commented Mar 21, 2018 • edited Loading

danielnelson commented Mar 21, 2018

nerzhul commented Mar 22, 2018

nerzhul commented Apr 24, 2018

nerzhul commented May 11, 2018

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

nerzhul commented May 14, 2018

nerzhul commented May 16, 2018

zaaraameera2014 left a comment

Choose a reason for hiding this comment

nerzhul commented May 16, 2018

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeseacher commented Jul 17, 2018

nerzhul commented Jul 17, 2018

nerzhul commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glinton commented Jul 24, 2018

nerzhul commented Jul 24, 2018

glinton commented Jul 24, 2018

nerzhul commented Jul 24, 2018

nerzhul commented Jul 24, 2018

nerzhul commented Aug 1, 2018

freeseacher commented Aug 1, 2018

glinton commented Aug 1, 2018

nerzhul commented Aug 2, 2018

nerzhul commented Mar 21, 2018 •

edited

Loading

nerzhul commented Mar 21, 2018 •

edited

Loading