database_observability: Add pgsql engine version to connection_info #4099
Conversation
…o downstream collectors
rgeyer
requested review from
cristiangreco,
matthewnolf and
fridgepoet
as code owners
|
I wonder why the version information includes build and os info... It might be important for debugging edge cases. Can we put it in a additional field. |
Did do, it now produces a result like this..
|
|
|
||
| args.Registry.MustRegister(infoMetric) | ||
|
|
||
| return &ConnectionInfo{ | ||
| DSN: args.DSN, | ||
| Registry: args.Registry, | ||
| DBVersion: args.DBVersion, |
There was a problem hiding this comment.
should we use the same engine terminology?
There was a problem hiding this comment.
Fair play. Updated that as well. In MySQL we might consider renaming that in the explain plan schema as well.
internal/component/database_observability/postgres/collector/connection_info.go
Outdated
Show resolved
Hide resolved
| @@ -58,6 +63,8 @@ func (c *ConnectionInfo) Start(ctx context.Context) error { | |||
| providerRegion = "unknown" | |||
| dbInstanceIdentifier = "unknown" | |||
| engine = "postgres" | |||
| engineVersion = "unknown" | |||
| engineVersionSuffix = "none" | |||
There was a problem hiding this comment.
Do we need the suffix for anything? If not I'd remove it for now (easier to add later when we need it)
There was a problem hiding this comment.
I asked Ryan to put this up since it won't hurt and this info might be useful for debugging in the future.
There was a problem hiding this comment.
I feel the opposite - let's not collect data we don't need. We can always add it later once we have a use for it.
internal/component/database_observability/postgres/collector/connection_info.go
Outdated
Show resolved
Hide resolved
|
|
||
| matches := versionRegex.FindStringSubmatch(c.EngineVersion) | ||
| fmt.Println(matches) | ||
| if len(matches) > 1 { |
There was a problem hiding this comment.
Should these be inclusive?
PR Description
More-or-less a mirror of #4097 to implement this same functionality for pgsql.
Notes to the Reviewer
The
SHOW server_versionis still fairly verbose, and provides not only the version, but also the OS flavor and build details (I.E.17.2 (Debian 17.2-1.pgdg120+1)).Do we care about the fine-grained detail, or do we only want the major version (17.2)?
My gut is to just parse this and drop everything after the major version number.