-
Notifications
You must be signed in to change notification settings - Fork 389
database_observability: Add pgsql engine version to connection_info #4099
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.
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
d61bc27
6e56dad
a25a378
58da06e
9f1b190
6b4d52a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package collector | |
|
||
import ( | ||
"context" | ||
"fmt" | ||
"regexp" | ||
"strings" | ||
|
||
|
@@ -12,19 +13,22 @@ import ( | |
const ConnectionInfoName = "connection_info" | ||
|
||
var ( | ||
rdsRegex = regexp.MustCompile(`(?P<identifier>[^\.]+)\.([^\.]+)\.(?P<region>[^\.]+)\.rds\.amazonaws\.com`) | ||
azureRegex = regexp.MustCompile(`(?P<identifier>[^\.]+)\.postgres\.database\.azure\.com`) | ||
rdsRegex = regexp.MustCompile(`(?P<identifier>[^\.]+)\.([^\.]+)\.(?P<region>[^\.]+)\.rds\.amazonaws\.com`) | ||
azureRegex = regexp.MustCompile(`(?P<identifier>[^\.]+)\.postgres\.database\.azure\.com`) | ||
versionRegex = regexp.MustCompile(`(?P<version>^[1-9]+\.[1-9]+)(?P<suffix>.*)?$`) | ||
) | ||
|
||
type ConnectionInfoArguments struct { | ||
DSN string | ||
Registry *prometheus.Registry | ||
DSN string | ||
Registry *prometheus.Registry | ||
EngineVersion string | ||
} | ||
|
||
type ConnectionInfo struct { | ||
DSN string | ||
Registry *prometheus.Registry | ||
InfoMetric *prometheus.GaugeVec | ||
DSN string | ||
Registry *prometheus.Registry | ||
EngineVersion string | ||
InfoMetric *prometheus.GaugeVec | ||
|
||
running *atomic.Bool | ||
} | ||
|
@@ -34,15 +38,16 @@ func NewConnectionInfo(args ConnectionInfoArguments) (*ConnectionInfo, error) { | |
Namespace: "database_observability", | ||
Name: "connection_info", | ||
Help: "Information about the connection", | ||
}, []string{"provider_name", "provider_region", "db_instance_identifier", "engine"}) | ||
}, []string{"provider_name", "provider_region", "db_instance_identifier", "engine", "engine_version", "engine_version_suffix"}) | ||
|
||
args.Registry.MustRegister(infoMetric) | ||
|
||
return &ConnectionInfo{ | ||
DSN: args.DSN, | ||
Registry: args.Registry, | ||
InfoMetric: infoMetric, | ||
running: &atomic.Bool{}, | ||
DSN: args.DSN, | ||
Registry: args.Registry, | ||
EngineVersion: args.EngineVersion, | ||
InfoMetric: infoMetric, | ||
running: &atomic.Bool{}, | ||
}, nil | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
) | ||
|
||
parts, err := ParseURL(c.DSN) | ||
|
@@ -81,7 +88,17 @@ func (c *ConnectionInfo) Start(ctx context.Context) error { | |
} | ||
} | ||
} | ||
c.InfoMetric.WithLabelValues(providerName, providerRegion, dbInstanceIdentifier, engine).Set(1) | ||
|
||
matches := versionRegex.FindStringSubmatch(c.EngineVersion) | ||
fmt.Println(matches) | ||
rgeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if len(matches) > 1 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these be inclusive? |
||
engineVersion = matches[1] | ||
} | ||
if len(matches) > 2 && matches[2] != "" { | ||
engineVersionSuffix = strings.TrimSpace(matches[2]) | ||
} | ||
|
||
c.InfoMetric.WithLabelValues(providerName, providerRegion, dbInstanceIdentifier, engine, engineVersion, engineVersionSuffix).Set(1) | ||
return nil | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,37 +17,50 @@ func TestConnectionInfo(t *testing.T) { | |
const baseExpectedMetrics = ` | ||
# HELP database_observability_connection_info Information about the connection | ||
# TYPE database_observability_connection_info gauge | ||
database_observability_connection_info{db_instance_identifier="%s",engine="%s",provider_name="%s",provider_region="%s"} 1 | ||
database_observability_connection_info{db_instance_identifier="%s",engine="%s",engine_version="%s",engine_version_suffix="%s",provider_name="%s",provider_region="%s"} 1 | ||
` | ||
|
||
testCases := []struct { | ||
name string | ||
dsn string | ||
expectedMetrics string | ||
name string | ||
dsn string | ||
engineVersion string | ||
engineVersionSuffix string | ||
expectedMetrics string | ||
}{ | ||
{ | ||
name: "generic dsn", | ||
dsn: "postgres://user:pass@localhost:5432/mydb", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "unknown", "postgres", "unknown", "unknown"), | ||
engineVersion: "15.4", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "unknown", "postgres", "15.4", "none", "unknown", "unknown"), | ||
}, | ||
{ | ||
name: "AWS/RDS dsn", | ||
dsn: "postgres://user:[email protected]:5432/mydb", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "products-db", "postgres", "aws", "us-east-1"), | ||
engineVersion: "15.4", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "products-db", "postgres", "15.4", "none", "aws", "us-east-1"), | ||
}, | ||
{ | ||
name: "Azure flexibleservers dsn", | ||
dsn: "postgres://user:[email protected]:5432/mydb", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "products-db", "postgres", "azure", "unknown"), | ||
name: "Azure flexibleservers dsn", | ||
dsn: "postgres://user:[email protected]:5432/mydb", | ||
engineVersion: "15.4", | ||
engineVersionSuffix: "none", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "products-db", "postgres", "15.4", "none", "azure", "unknown"), | ||
}, | ||
{ | ||
name: "version suffix", | ||
dsn: "postgres://user:pass@localhost:5432/mydb", | ||
engineVersion: "15.4 (Debian 15.4-1.pgdg120+1)", | ||
expectedMetrics: fmt.Sprintf(baseExpectedMetrics, "unknown", "postgres", "15.4", "(Debian 15.4-1.pgdg120+1)", "unknown", "unknown"), | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
reg := prometheus.NewRegistry() | ||
|
||
collector, err := NewConnectionInfo(ConnectionInfoArguments{ | ||
DSN: tc.dsn, | ||
Registry: reg, | ||
DSN: tc.dsn, | ||
Registry: reg, | ||
EngineVersion: tc.engineVersion, | ||
}) | ||
require.NoError(t, err) | ||
require.NotNil(t, collector) | ||
|
Uh oh!
There was an error while loading. Please reload this page.