Skip to content
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

postgresql receiver inconsistently does not include schemaname in tablename for some metrics #29559

Closed
cultpony opened this issue Nov 29, 2023 · 9 comments
Labels
bug Something isn't working receiver/postgresql Stale

Comments

@cultpony
Copy link
Contributor

Component(s)

receiver/postgresql

What happened?

Description

When retreiving the metrics for index and sequential scans (postgresql.sequential_scan and postgresql.index.scan), the tablename in the metricset is inconsistently constructed;

Sequential Scan Metrics will concat the schema to the tablename (schema.relation), while Index.Scan will only use the Table Name, omitting the schema (relation)

You can see the issue here in these two codeblocks:

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.88.0/receiver/postgresqlreceiver/client.go#L270

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.88.0/receiver/postgresqlreceiver/client.go#L376

The same occurs with locks, which also do not include a schema.

The issue is that as a result, it becomes harder to actually correlate between the datasets, especially if tables exists with the same name in two schemas.

Steps to Reproduce

  1. Enable postgresql Metrics with optional metrics such as postgresql.sequential_scans

Expected Result

Metrics have consistent names for the resources they track.

Actual Result

Metrics have inconsistent names and cannot be reliably correlated.

Collector version

v0.88.0, v0.90.0, 3b84eae

Environment information

not relevant

OpenTelemetry Collector configuration

receivers:
  postgresql:
    endpoint: "localhost:5432"
    username: postgres
    password: postgres
    metrics:
      postgresql.sequential_scans:
        enabled: true

Log output

not relevant

Additional context

I'm happy to write a patch to change the index.* metrics to use a "schema.relation" name mapping in the same way all the other metrics for postgres work. It would be nice if all metrics had consistent tagging.

@cultpony cultpony added bug Something isn't working needs triage New item requiring triage labels Nov 29, 2023
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@djaglowski
Copy link
Member

Thanks for pointing this out @cultpony. I agree we should be consistent across the receiver.

We seem to have two options here. Either have postgresql.database.name: "schema.relation", or add a new attribute postgresql.schema.name and store the fields separately.

Generally I lean towards using separate attributes when we have two pieces of information. WDYT?

@cultpony
Copy link
Contributor Author

Using two attributes would be definitely the better choice, IMO

@crobert-1
Copy link
Member

Removing needs triage as the next steps and solution has been agreed upon.

@hughesjj
Copy link
Contributor

Made a comment in the PR, but have we thought about backwards compatibility/migration path for anyone reliant on the old naming scheme?

@cultpony
Copy link
Contributor Author

I mentioned it in the PR (and to bring this on the issue so that discussion isn't fragmented too much) but the best option would be to recommend users to migrate their system to rely on the newer attributes, since they are simply more reliable.

To restore old behaviour a transform step in the pipeline would be sufficient:

transform:
  error_mode: ignore
  metric_statements:
  - context: resource
    statements:
    - set(attributes["postgresql.table.name"], Concat(attributes["postgresql.table.schema"],attributes["postgresql.table.name"],"."))

@djaglowski
Copy link
Member

Thanks @hughesjj for bringing up backwards compatibility, and @cultpony for putting together the PR. I saw the PR first so commented there, but really our backwards compatibility policy requires us to make breaking changes like this via a feature gate. That said, it's very helpful to have the corresponding transform for those who wish to keep the old format longer term, so thanks for putting that together as well.

djaglowski pushed a commit that referenced this issue Jan 22, 2024
…ver (#30142)

**Description:** 

Changes the postgresql statistics receiver so that a table or indices
schema is stored in it's own attribute.

Currently the schema is stored as table name for *some* of the
statistics, while others completely ignore what schema a statistic comes
out of.

The code will allow computing things like sequential vs index scans
against a table without having to pick apart the resource attribute.

**Link to tracking Issue:**  #29559 

**Testing:** I've built the docker container for otel contrib and have
been running the result for a few days to observe that the attributes
are properly propagated.

**Documentation:** Documentation for the new field was added to the
metadata.yaml and documentation.md
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Feb 1, 2024
…ver (open-telemetry#30142)

**Description:** 

Changes the postgresql statistics receiver so that a table or indices
schema is stored in it's own attribute.

Currently the schema is stored as table name for *some* of the
statistics, while others completely ignore what schema a statistic comes
out of.

The code will allow computing things like sequential vs index scans
against a table without having to pick apart the resource attribute.

**Link to tracking Issue:**  open-telemetry#29559 

**Testing:** I've built the docker container for otel contrib and have
been running the result for a few days to observe that the attributes
are properly propagated.

**Documentation:** Documentation for the new field was added to the
metadata.yaml and documentation.md
anthoai97 pushed a commit to anthoai97/opentelemetry-collector-contrib that referenced this issue Feb 12, 2024
…ver (open-telemetry#30142)

**Description:** 

Changes the postgresql statistics receiver so that a table or indices
schema is stored in it's own attribute.

Currently the schema is stored as table name for *some* of the
statistics, while others completely ignore what schema a statistic comes
out of.

The code will allow computing things like sequential vs index scans
against a table without having to pick apart the resource attribute.

**Link to tracking Issue:**  open-telemetry#29559 

**Testing:** I've built the docker container for otel contrib and have
been running the result for a few days to observe that the attributes
are properly propagated.

**Documentation:** Documentation for the new field was added to the
metadata.yaml and documentation.md
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Mar 11, 2024
@crobert-1
Copy link
Member

Resolved by #30142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working receiver/postgresql Stale
Projects
None yet
Development

No branches or pull requests

4 participants