apps/postgresql: Receiver Enhancements (Metric Additions) #804
Conversation
…o postgres-enhancements-v2
…o postgres-enhancements-v2
…o postgres-enhancements-v2
…o postgres-enhancements-v2
There was a problem hiding this comment.
Compared the implementation with the scope doc. There are a few discrepancies that are not unclear whether they are intentional or by accident. We can discuss them case by case. See detailed comments.
The main concern is:
- Are the newly added labels gonna cause granularity / scalability burden that is worth revisiting.
- If there were some proposals to change what metrics should look like, the suggestion should be recorded in the scope doc with reasoning.
integration_test/third_party_apps_data/applications/postgresql/metadata.yaml
Show resolved
Hide resolved
| kind: GAUGE | ||
| - kind: CUMULATIVE | ||
| monitored_resource: gce_instance | ||
| type: workload.googleapis.com/postgresql.bgwriter.maxwritten |
There was a problem hiding this comment.
The scope doc has this as workload.googleapis.com/postgresql.bgwriter.maxwriten.count
Is this an intentional change to the scope doc? If so, can we send a suggested edit to the scope doc and have the approver double check it? If not, can we fix the translation so it matches the scope doc?
There was a problem hiding this comment.
Intentional change, due to this open-telemetry/opentelemetry-collector-contrib#13328 (comment)
Will make a suggestion
integration_test/third_party_apps_data/applications/postgresql/metadata.yaml
Show resolved
Hide resolved
| value_type: INT64 | ||
| kind: GAUGE | ||
| - kind: CUMULATIVE | ||
| labels: |
There was a problem hiding this comment.
For workload.googleapis.com/postgresql.index.scans, the scope doc does not include any labels.
Is this an intentional change to the scope doc? If so, can we send a suggested edit to the scope doc and have the approver double check it? If not, can we fix the translation so it matches the scope doc?
There was a problem hiding this comment.
We needed to identify the index with labels for its size. Will make a suggestion to scope doc.
integration_test/third_party_apps_data/applications/postgresql/metadata.yaml
Show resolved
Hide resolved
integration_test/third_party_apps_data/applications/postgresql/metadata.yaml
Show resolved
Hide resolved
| labels: | ||
| database: .* | ||
| monitored_resource: gce_instance | ||
| type: workload.googleapis.com/postgresql.table.count |
There was a problem hiding this comment.
The scope doc does not include labels for workload.googleapis.com/postgresql.table.count
Is this an intentional change to the scope doc? If so, can we send a suggested edit to the scope doc and have the approver double check it? If not, can we fix the translation so it matches the scope doc?
There was a problem hiding this comment.
Databases give context to the table count. Made a suggestion to the scope doc.
| database: .* | ||
| table: .* | ||
| monitored_resource: gce_instance | ||
| type: workload.googleapis.com/postgresql.table.size |
There was a problem hiding this comment.
The scope doc include no labels for workload.googleapis.com/postgresql.table.size
Is this an intentional change to the scope doc? If so, can we send a suggested edit to the scope doc and have the approver double check it? If not, can we fix the translation so it matches the scope doc?
There was a problem hiding this comment.
It needs these labels to be properly identified per table. Will make a suggestion to the scope doc.
integration_test/third_party_apps_data/applications/postgresql/metadata.yaml
Show resolved
Hide resolved
integration_test/third_party_apps_data/applications/postgresql/metadata.yaml
Show resolved
Hide resolved
|
CC @braydonk for shadowing this PR review. |
I don't believe that the newly added labels should be a large cause of concern. The most nested would be a metric datapoint with a value for
I submitted a bunch of suggestions based off what I found. Let me know who to ping or if further clarification is needed on them. |
|
Looks like a flake to me. Re-running to make sure. |
|
|
Description
receiver.postgresql.emitMetricsWithResourceAttributesfor more information on transitioning, please reference https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/postgresqlreceiver#transition-scheduleRelated issue
How has this been tested?
Integration testing and updates to confgenerator unit testing
Checklist: