Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance postgres service to understand pgbouncer data types #6022

Closed
wants to merge 4 commits into from
Closed

Enhance postgres service to understand pgbouncer data types #6022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98d7f7e
Merge pull request #1 from influxdata/master
johnatannvmd Jun 17, 2019
c4094ac
Enhance postgres data types
Jun 18, 2019
4e32d7d
Remove pgbounce specific
Jun 20, 2019
1453a48
Change README.md according review
Jun 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions plugins/inputs/pgbouncer/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,29 @@ This PgBouncer plugin provides metrics for your PgBouncer load balancer.

More information about the meaning of these metrics can be found in the [PgBouncer Documentation](https://pgbouncer.github.io/usage.html)

## Reproduce behaviour with postgresql_extensible input

```
[[inputs.postgresql_extensible]]
address = "host=localhost port=6432 user=postgres password=postgres sslmode=disable dbname=pgbouncer"
[[inputs.postgresql_extensible.query]]
measurement = "pgbouncer"
sqlquery = "show stats;"
withdbname = false
[[inputs.postgresql_extensible.query]]
measurement = "pgbouncer_pools"
sqlquery = "show pools;"
withdbname = false
```

Output metrics will be the same. Also you can use any of this queries to expose internal data:
```
SHOW STATS;
SHOW SERVERS;
SHOW CLIENTS;
SHOW POOLS;
```

## Configuration
Specify address via a postgresql connection string:

Expand Down
1 change: 0 additions & 1 deletion plugins/inputs/pgbouncer/pgbouncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ func init() {
MaxLifetime: internal.Duration{
Duration: 0,
},
IsPgBouncer: true,
},
}
})
Expand Down
1 change: 0 additions & 1 deletion plugins/inputs/pgbouncer/pgbouncer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ func TestPgBouncerGeneratesMetrics(t *testing.T) {
"host=%s user=pgbouncer password=pgbouncer dbname=pgbouncer port=6432 sslmode=disable",
testutil.GetLocalHost(),
),
IsPgBouncer: true,
},
}

Expand Down
1 change: 0 additions & 1 deletion plugins/inputs/postgresql/postgresql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ func init() {
MaxLifetime: internal.Duration{
Duration: 0,
},
IsPgBouncer: false,
},
}
})
Expand Down
1 change: 0 additions & 1 deletion plugins/inputs/postgresql/postgresql_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ func TestPostgresqlGeneratesMetrics(t *testing.T) {
"host=%s user=postgres sslmode=disable",
testutil.GetLocalHost(),
),
IsPgBouncer: false,
},
Databases: []string{"postgres"},
}
Expand Down
63 changes: 39 additions & 24 deletions plugins/inputs/postgresql/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ type Service struct {
MaxOpen int
MaxLifetime internal.Duration
DB *sql.DB
IsPgBouncer bool
}

// Start starts the ServiceInput's service, whatever that may be
Expand All @@ -104,34 +103,50 @@ func (p *Service) Start(telegraf.Accumulator) (err error) {
p.Address = localhost
}

connectionString := p.Address

// Specific support to make it work with PgBouncer too
// See https://github.com/influxdata/telegraf/issues/3253#issuecomment-357505343
if p.IsPgBouncer {
d := &stdlib.DriverConfig{
ConnConfig: pgx.ConnConfig{
PreferSimpleProtocol: true,
RuntimeParams: map[string]string{
"client_encoding": "UTF8",
},
CustomConnInfo: func(c *pgx.Conn) (*pgtype.ConnInfo, error) {
info := c.ConnInfo.DeepCopy()
info.RegisterDataType(pgtype.DataType{
Value: &pgtype.OIDValue{},
Name: "int8OID",
OID: pgtype.Int8OID,
})

return info, nil
},
// Enhanced pgx data types for fully working with any pgbouncer request
Copy link
Contributor

@james-lawrence james-lawrence Jun 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnatannvmd @danielnelson sure, its been awhile though I'm a little rusty/out of date on pgx internals. personally I'd avoid combining the configurations entirely. because then we may run into subtle implementation details of info.RegisterDataType there is also very little benefit to doing so code/maintenance wise. I believe this is what has daniel concerned, and I agree, I'm willing to be convienced though, but it'll require explicit code samples from pgx and tests in this repo to convince me.

I think the current code in master is pretty close to what I'd do regardless. with some minor improvements. This doesn't let you use postgresql extensible against pgbouncer directly. but i think that is a good thing. less complexity.

what I'd do:

  • change p.IsPgBounder to pgx.ConnConfig, and check for nil/zero if necessary, depends on the mechanics of merge. means other postgresql wire protocols will work here without issue.
  • move the pgx.ConnConfig into the pgbouncer package (its pgbouncer specific)
  • update connection stuff here to use the more recent features in pgx. (stdlib.OpenDB)
// plugins/inputs/postgresql/service.go

// Start starts the ServiceInput's service, whatever that may be
func (p *Service) Start(telegraf.Accumulator) (err error) {
	const localhost = "host=localhost sslmode=disable"
        var (
             connConfig pgx.ConnConfig
        )

	if p.Address == "" || p.Address == "localhost" {
		p.Address = localhost
        }

      	if connConfig, err = pgx.ParseConnectionString(p.Address); err != nil {
		return err
	}

        p.DB = stdlib.OpenDB(connConfig.Merge(t.ConnConfig))
      
        return nil
}
// plugins/inputs/pgbouncer/pgbouncer.go
func connConfig() pgx.ConnConfig {
       return pgx.ConnConfig{
          // ...
       }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it'll require explicit code samples from pgx and tests in this repo to convince me.

I totally agree with you. And I will prepare code samples from gpx, Postgres and pgbounce about return types and how the response is prepared.

The reason for these changes is to make possible to query different information from pgbouncer, not only SHOW POOLS and SHOW STATS. But I totally understand what you want:

move the pgx.ConnConfig into the pgbouncer package (its pgbouncer specific)
is a good point for less complexity and division of responsibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for these changes is to make possible to query different information from pgbouncer, not only SHOW POOLS and SHOW STATS

I understood the goals of the PR. =)

if you follow the suggestions I offered I believe pgbouncer can actually become a simple configuration setup over postgres_extensible. if we so desire.

driverConfig := &stdlib.DriverConfig{
ConnConfig: pgx.ConnConfig{
PreferSimpleProtocol: true,
RuntimeParams: map[string]string{
"client_encoding": "UTF8",
},
}
stdlib.RegisterDriverConfig(d)
connectionString = d.ConnectionString(p.Address)
CustomConnInfo: func(c *pgx.Conn) (*pgtype.ConnInfo, error) {
info := c.ConnInfo.DeepCopy()
info.RegisterDataType(pgtype.DataType{
Value: &pgtype.OIDValue{},
Name: "int8OID",
OID: pgtype.Int8OID,
})
info.RegisterDataType(pgtype.DataType{
Value: &pgtype.OIDValue{},
Name: "zerroTypeOID",
OID: 0,
})
info.RegisterDataType(pgtype.DataType{
Value: &pgtype.OIDValue{},
Name: "numericTypeOID",
OID: pgtype.NumericOID,
})
info.RegisterDataType(pgtype.DataType{
Value: &pgtype.OIDValue{},
Name: "float8TypeOID",
OID: pgtype.Float8OID,
})
info.RegisterDataType(pgtype.DataType{
Value: &pgtype.OIDValue{},
Name: "timestamptzTypeOID",
OID: pgtype.TimestamptzOID,
})

return info, nil
},
},
}
stdlib.RegisterDriverConfig(driverConfig)

if p.DB, err = sql.Open("pgx", connectionString); err != nil {
if p.DB, err = sql.Open("pgx", driverConfig.ConnectionString(p.Address)); err != nil {
return err
}

Expand Down
1 change: 0 additions & 1 deletion plugins/inputs/postgresql_extensible/postgresql_extensible.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@ func init() {
MaxLifetime: internal.Duration{
Duration: 0,
},
IsPgBouncer: false,
},
}
})
Expand Down
1 change: 0 additions & 1 deletion plugins/inputs/postgresql_extensible/postgresql_extensible_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ func queryRunner(t *testing.T, q query) *testutil.Accumulator {
"host=%s user=postgres sslmode=disable",
testutil.GetLocalHost(),
),
IsPgBouncer: false,
},
Databases: []string{"postgres"},
Query: q,
Expand Down