Add cloud_provider block to database_observability.postgres
#4675
Conversation
|
💻 Deploy preview deleted (Add |
cristiangreco
requested review from
a team and
clayton-cornell
as code owners
Copy over functionality from `database_observability.mysql` to populate cloud provider info in `connection_info` collector for Postgres.
cristiangreco
force-pushed
the
cristian/dbo11y-postgres-cloud-provider-info
branch
from
b226d37 to
d53996f
Compare
| }, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
A bunch of copy-paste from database_observability.mysql, but I think it's fine for now.
There was a problem hiding this comment.
Pull Request Overview
This PR adds cloud provider configuration support to the database_observability.postgres component by introducing a cloud_provider block with AWS-specific information. This functionality mirrors what already exists in the MySQL database observability component.
Key changes:
- Added
cloud_providerandawsconfiguration blocks to accept AWS ARN information - Updated
connection_infocollector to use cloud provider data and addprovider_accountlabel - Enhanced test coverage to validate cloud provider information handling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/component/database_observability/postgres/component.go | Adds CloudProvider configuration structures and parses AWS ARN to pass to connection_info collector |
| internal/component/database_observability/postgres/collector/connection_info.go | Updates connection_info collector to accept and use cloud provider info, adding provider_account label |
| internal/component/database_observability/postgres/collector/connection_info_test.go | Adds test case for cloud provider info and updates expected metrics to include provider_account label |
| internal/component/database_observability/mysql/collector/connection_info.go | Reorders imports (formatting change only) |
| docs/sources/reference/components/database_observability/database_observability.postgres.md | Documents the new cloud_provider and aws configuration blocks with usage examples |
| arn, err := arn.Parse(c.args.CloudProvider.AWS.ARN) | ||
| if err != nil { | ||
| level.Error(c.opts.Logger).Log("msg", "failed to parse AWS cloud provider ARN", "err", err) | ||
| } | ||
| cloudProviderInfo = &database_observability.CloudProvider{ | ||
| AWS: &database_observability.AWSCloudProviderInfo{ | ||
| ARN: arn, | ||
| }, | ||
| } |
There was a problem hiding this comment.
When ARN parsing fails, the code logs an error but continues to use the uninitialized arn variable in the CloudProvider struct (line 362). This will result in an empty ARN being passed to the collector. Either return early after logging the error, or skip setting cloudProviderInfo when parsing fails.
| arn, err := arn.Parse(c.args.CloudProvider.AWS.ARN) | |
| if err != nil { | |
| level.Error(c.opts.Logger).Log("msg", "failed to parse AWS cloud provider ARN", "err", err) | |
| } | |
| cloudProviderInfo = &database_observability.CloudProvider{ | |
| AWS: &database_observability.AWSCloudProviderInfo{ | |
| ARN: arn, | |
| }, | |
| } | |
| parsedARN, err := arn.Parse(c.args.CloudProvider.AWS.ARN) | |
| if err != nil { | |
| level.Error(c.opts.Logger).Log("msg", "failed to parse AWS cloud provider ARN", "err", err) | |
| } else { | |
| cloudProviderInfo = &database_observability.CloudProvider{ | |
| AWS: &database_observability.AWSCloudProviderInfo{ | |
| ARN: parsedARN, | |
| }, | |
| } | |
| } |
| if c.CloudProvider != nil { | ||
| if c.CloudProvider.AWS != nil { | ||
| providerName = "aws" | ||
| matches := rdsRegex.FindStringSubmatch(host) | ||
| if len(matches) > 3 { | ||
| dbInstanceIdentifier = matches[1] | ||
| providerRegion = matches[3] | ||
| providerAccount = c.CloudProvider.AWS.ARN.AccountID | ||
| providerRegion = c.CloudProvider.AWS.ARN.Region | ||
|
|
||
| // We only support RDS database for now. Resource types and ARN formats are documented at: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonrds.html#amazonrds-resources-for-iam-policies | ||
| if resource := c.CloudProvider.AWS.ARN.Resource; strings.HasPrefix(resource, "db:") { | ||
| dbInstanceIdentifier = strings.TrimPrefix(resource, "db:") | ||
| } | ||
| } else if strings.HasSuffix(host, "postgres.database.azure.com") { | ||
| providerName = "azure" | ||
| matches := azureRegex.FindStringSubmatch(host) | ||
| if len(matches) > 1 { | ||
| dbInstanceIdentifier = matches[1] | ||
| } | ||
| } else { |
There was a problem hiding this comment.
[nitpick] The logic for determining connection metadata is split between cloud provider configuration (lines 72-83) and DSN parsing (lines 84-103). When cloud provider info is available, DSN-based detection is completely bypassed. Consider whether DSN parsing should be used as a fallback for missing cloud provider fields (like region or instance identifier) rather than being mutually exclusive, or add a comment explaining why this all-or-nothing approach is intentional.
|
Interesting CoPilot review. I was looking to see if it'd comment on the doc input. |
3 participants
PR Description
Copy over functionality from
database_observability.mysqlto populate cloud provider info inconnection_infocollector for Postgres.Which issue(s) this PR fixes
n.a.
Notes to the Reviewer
PR Checklist