-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[receiver/postgresql] Add service.instance.id resource attribute to metrics and logs #43913
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?
Conversation
| if err != nil { | ||
| return fallback | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed because net.SplitHostPort(config.Endpoint) doesn't pass validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmitryax do you mean config.Endpoint will always be valid and so we wouldn't need a fallback ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use server.address and server.port? I see it's defined in semconv for some databases like OracleDB while service.instance.id isn't mentioned
I won't object to have Writing aggregation queries on a metric system can be painful when dynamic resource attribute filtering is required across different system types. In this context, This approach also aligns with prometheus receiver where it uses
This is a semconv for client spans. It is a good reference but there is no such a semconv for data fetched from database servers that a database receiver can use. 😢 |
| enabled: true | ||
| type: string | ||
| service.instance.id: | ||
| description: A unique identifier of the PostgreSQL instance in the format host:port (defaults to 'unknown:5432' in case of error in generating this value). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look how this could work for K8s deployments of MySQL and the support of the specs Specify resource attributes using Kubernetes annotations related to service.instance.id?
I could imagine the OTel Collector Receiver Creator being capable of generating service.instance.id from the specs and then passing it to the mysql receiver config.
|
@XSAM you might find this discussion interesting: open-telemetry/semantic-conventions#2147 (comment) (suggestion to capture Oracel instance name in a dedicated attribute) I think #43913 (comment) brings an important point: receiver does not know in which context / environment database runs, resource detectors do. Postgres receiver can't support k8s, docker, cloud provider-specific, etc resource detection, and if it populates service.instance.id on postgres metrics using service address and port, it won't match instance on logs or host metrics by the resource detection processor. Do we know if resource detection processor overrides Would it be reasonable to set server.address and server.port as well as any other attributes identifying databased instance within a host as dedicated attributes and let resource detectors populate |
Description
The service.instance.id attribute is added into resourceAttributes of metrics and logs to uniquely identify
PostgreSQL hosts. The format is :. And defaults to 'unknown:5432' in case of any errors in constructing this value.
Link to tracking issue
Fixes #43907
Testing
Unit tests added/updated
Documentation
Updated