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

Avoid adding sql mem metrics twice #47750

Merged
merged 1 commit into from
May 20, 2020

Conversation

jsoriano
Copy link
Contributor

SQL memory metrics are registered twice and they are also reported
twice by metrics exporters. This is specially strange on Prometheus
histograms, where buckets are duplicated:

sql_mem_sql_txn_max_bucket{le="+Inf"} 0.0
sql_mem_sql_txn_max_sum 0.0
sql_mem_sql_txn_max_count 0.0
sql_mem_sql_txn_max_bucket{le="+Inf"} 0.0
sql_mem_sql_txn_max_sum 0.0
sql_mem_sql_txn_max_count 0.0

Release note (bug fix): Remove duplicated SQL memory metrics.

SQL memory metrics are registered twice and they are also reported
twice by metrics exporters. This is specially strange on Prometheus
histograms, where buckets are duplicated:

  sql_mem_sql_txn_max_bucket{le="+Inf"} 0.0
  sql_mem_sql_txn_max_sum 0.0
  sql_mem_sql_txn_max_count 0.0
  sql_mem_sql_txn_max_bucket{le="+Inf"} 0.0
  sql_mem_sql_txn_max_sum 0.0
  sql_mem_sql_txn_max_count 0.0

Release note (bug fix): Remove duplicated SQL memory metrics.
@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Apr 21, 2020

CLA assistant check
All committers have signed the CLA.

@blathers-crl
Copy link

blathers-crl bot commented Apr 21, 2020

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

  • A person you worked with closely on this PR.
  • The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
  • Join our community slack channel and ask on #contributors.
  • Try find someone else from here.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@@ -469,8 +469,8 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) {
execCfg.StatsRefresher = statsRefresher

// Set up internal memory metrics for use by internal SQL executors.
// Don't add them to the registry now because it will be added as part of pgServer metrics.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here:

for _, m := range pgServer.Metrics() {
cfg.registry.AddMetricStruct(m)
}

@jsoriano
Copy link
Contributor Author

Hey @RaduBerinde, I have seen you have been working on some SQL telemetry fixes recently. Maybe you can review this PR? 🙂 Thanks!

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@jsoriano
Copy link
Contributor Author

@RaduBerinde thanks for the review, do you know if there is anything else needed to have this merged? 🙂

@RaduBerinde
Copy link
Member

Oh, sorry, merging now.

bors r+

@craig
Copy link
Contributor

craig bot commented May 20, 2020

Build succeeded

@craig craig bot merged commit 31656ed into cockroachdb:master May 20, 2020
@SWDevAngel
Copy link

@jsoriano Hi Jaime. Thank you for contributing to CockroachDB this year. As a token of our appreciation we would like to send you a gift. Please DM me on our community Slack @lisa so I can send you a link. (If you don’t want a gift, we also have a charitable contribution choice.) All orders need to be in by December 13, so please send this asap. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants