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

Correct PostgreSQL metrics for dead tuples, with integration tests and refactoring #2474

Conversation

easimon
Copy link
Contributor

@easimon easimon commented Feb 24, 2021

Larger version of #2468

  • Corrected PostgreSQL metrics for dead tuples
  • Refactored PostgreSQL metrics, extracted String constants for less repetitions.
  • Added integration tests to verify (esp.) the dead rows gauge.
  1. pg_stat_user_tables has a row for each table in the database, so selecting the first row of it results in "dead rows for a single random table". Calculating the sum over n_dead_tup results in all dead rows instead.

  2. pg_stat_database has only one row for each database, so the sum() over numbackends was harmless but unnecessary, so I removed that one.

  3. At least for PostgreSQL, there's a difference between getObject(n, Long.class) and getLong(n). The former can only convert INT4 and INT8 (but not NUMERIC and DECIMAL) to Long, the latter converts all numeric types. sum(n_dead_tup) is such a case where this matters, since its type is NUMERIC.

I could understand if someone finds this too much of a change, so I created both MRs.

@easimon easimon force-pushed the fix/postgres-metrics-integration-test branch from 1b85474 to dd9d275 Compare February 24, 2021 10:33
@easimon easimon force-pushed the fix/postgres-metrics-integration-test branch 2 times, most recently from 4274a47 to 42ec92d Compare February 24, 2021 10:52
@shakuzen shakuzen added bug A general bug module: micrometer-core An issue that is related to our core module labels Jan 13, 2022
@shakuzen shakuzen added this to the 1.7.9 milestone Jan 13, 2022
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you for finding the issue, sending a fix, and adding integration tests for us. Much appreciated. I will rebase the changes on our 1.7.x branch so that we can include it in our next round of patch releases.

@shakuzen shakuzen force-pushed the fix/postgres-metrics-integration-test branch from 42ec92d to 48baa4f Compare January 13, 2022 11:37
@shakuzen shakuzen changed the base branch from main to 1.7.x January 13, 2022 11:37
@shakuzen shakuzen merged commit 6bd8645 into micrometer-metrics:1.7.x Jan 13, 2022
@shakuzen
Copy link
Member

Sorry for the long wait to get this in. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants