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

Set a default statement timeout for postgres to 5s #9847

Merged
merged 8 commits into from
Aug 6, 2021

Conversation

justiniso
Copy link
Contributor

@justiniso justiniso commented Aug 4, 2021

What does this PR do?

This PR updates the Postgres integration to set a default timeout of 5s on queries. Based on internal benchmarking, this should be a wide margin to ensure we mitigate performance impact in the worst case scenarios, while still ensuring customers have metrics.

Motivation

Datadog strives to provide high fidelity telemetry while guaranteeing the lowest possible performance impact by the agent. While the integrations do not run any queries that should exceed a 5s execution time, there are certain failure scenarios on databases that could result in all queries being slow and consuming more resources than a monitoring tool should–especially on a loaded database.

Additional Notes

I believe the biggest risk in changing this behavior is the impact on long-running custom queries. However the benefit to risk ratio is very hight.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@justiniso justiniso requested a review from a team as a code owner August 4, 2021 18:44
@justiniso justiniso marked this pull request as draft August 4, 2021 18:44
@ghost ghost added the integration/postgres label Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #9847 (0fd772e) into master (a0ede59) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Flag Coverage Δ
postgres 92.16% <100.00%> (+0.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@justiniso justiniso changed the title Set a default statement timeout for postgres Set a default statement timeout for postgres to 2s Aug 4, 2021
@ghost ghost added the documentation label Aug 4, 2021
@justiniso justiniso marked this pull request as ready for review August 5, 2021 00:00
@justiniso justiniso requested a review from a team as a code owner August 5, 2021 00:00
@justiniso justiniso force-pushed the justiniso/statement-timeout-postgres branch from 5eaa19a to 3c28131 Compare August 5, 2021 14:30
@justiniso justiniso changed the title Set a default statement timeout for postgres to 2s Set a default statement timeout for postgres to 5s Aug 5, 2021
postgres/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
ruthnaebeck
ruthnaebeck previously approved these changes Aug 5, 2021
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

Co-authored-by: Ofek Lev <ofekmeister@gmail.com>
@justiniso justiniso merged commit 6977867 into master Aug 6, 2021
@justiniso justiniso deleted the justiniso/statement-timeout-postgres branch August 6, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants