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

Change DB utils behavior when a truncated row is found to only drop the row #7983

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

justiniso
Copy link
Contributor

What does this PR do?

For database stats tables, this changes the behavior when a truncated row is found. Because this function computes the difference in monotonic counters between check runs, a value lower than the previous check run is assumed to be invalid. Currently, all rows are dropped when a single row is negative. This can result in states where all rows are dropped every check run due to bad state on a single row.

This was an edge case we caught early on, but forgot about when I opened the PR. I added a comment to avoid making the same mistake again.

Motivation

Additional Notes

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

@codecov
Copy link

codecov bot commented Nov 9, 2020

@justiniso justiniso force-pushed the justiniso/statement-metrics-fix-dropped-metrics branch from 86490d6 to ff156cb Compare November 10, 2020 00:19
@justiniso justiniso marked this pull request as ready for review November 10, 2020 00:36
@justiniso justiniso changed the title Change behavior when a truncated row is found to only drop the row Change DB utils behavior when a truncated row is found to only drop the row Nov 10, 2020
florimondmanca
florimondmanca previously approved these changes Nov 10, 2020
Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

LGTM

Should this be changelog/Fixed at least? There are code and behavioral changes so this needs to appear on the changelog.

@florimondmanca
Copy link
Contributor

CockroachDB pyXY-latest jobs are failing. There's probably a change in the latest version of the Cockroach Docker image. Maybe the port got moved, or the Docker container aren't starting up correctly due to a config change, etc. Irrelevant to this PR so we can merge, but this will need to be dealt with…

______________________ ERROR at setup of test_integration ______________________
tests/conftest.py:18: in dd_environment
    with docker_run(os.path.join(DOCKER_DIR, 'docker-compose.yaml'), endpoints=instance['prometheus_url']):
/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/contextlib.py:113: in __enter__
    return next(self.gen)
../datadog_checks_dev/datadog_checks/dev/docker.py:208: in docker_run
    with environment_run(
/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/contextlib.py:113: in __enter__
    return next(self.gen)
../datadog_checks_dev/datadog_checks/dev/env.py:73: in environment_run
    condition()
../datadog_checks_dev/datadog_checks/dev/conditions.py:84: in __call__
    raise RetryError('Endpoint: {}\n' 'Error: {}'.format(last_endpoint, last_error))
E   datadog_checks.dev.errors.RetryError: Endpoint: http://localhost:8080/_status/vars
E   Error: <urlopen error [Errno 111] Connection refused>

@justiniso
Copy link
Contributor Author

@florimondmanca I wasn't sure. If the changelog is for every version of integrations-core, then it should be changelog/Fixed. But if it is meant to be a public changelog for customers of the agent, there is no difference between the previous version of the agent and this one because the functionality has not been released yet.

@florimondmanca
Copy link
Contributor

@justiniso Fair call… In theory the changelog is for users, but in practice it's useful to list all PRs that changed code, even if technically they don't have customer impact (eg because they haven't been exposed to a bug because it had not been released yet). If we need to go back and investigate things then we'll be able to see that there was an initial PR, then one that fixed something missing.

@ofek ofek merged commit 4d14ac0 into master Nov 10, 2020
@ofek ofek deleted the justiniso/statement-metrics-fix-dropped-metrics branch November 10, 2020 15:07
ofek pushed a commit that referenced this pull request Nov 10, 2020
…he row (#7983)

* Change behavior when a truncated row is found to only drop the row

* Fix truncation tests
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