Fix tests that expect DBMAsync to put thread to sleep in sync mode #17847

kozlovb · 2024-06-14T10:10:27Z

What does this PR do?

Fixes tests that relied on main thread being put to sleep by DBMAsync.
Tests started to fail after this PR - #17716 got submitted.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Changelog entries must be created for modifications to shipped code
Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

codecov · 2024-06-14T10:13:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.16%. Comparing base (9a6a02d) to head (85aa002).
Report is 17 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
activemq	`?`
cassandra	`?`
hive	`?`
hivemq	`?`
ignite	`?`
jboss_wildfly	`?`
kafka	`?`
mysql	`87.84% <100.00%> (+4.93%)`	⬆️
postgres	`86.20% <100.00%> (+9.34%)`	⬆️
presto	`?`
solr	`?`
sqlserver	`88.87% <100.00%> (+6.67%)`	⬆️

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

github-actions · 2024-06-17T10:00:17Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2024-06-17T10:05:00Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2024-06-17T10:33:26Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2024-06-17T10:38:45Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions · 2024-06-17T10:50:32Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

kozlovb · 2024-06-17T10:58:15Z

sqlserver/tests/test_statements.py

@@ -311,10 +314,12 @@ def test_statement_metrics_and_plans(
    # 2) load the test queries into the StatementMetrics state
    # 3) emit the query metrics based on the diff of current and last state
    dd_run_check(check)
+    time.sleep(2)


why do we need sleep here? I thought we need either sleep or a close to zero collection interval but not both

I don't have an answer to this question yet.

What I know is that only in this test collection interval was set to 2s instead of 0.1 as in other tests.
link - https://github.com/DataDog/integrations-core/blob/13b7646c076108d76194c67e80c57515564fd489/sqlserver/tests/test_statements.py#L305
Also, if I reset my change in the base package and run this test with 0.1 collection interval it fails
branch - bk/test-sleep-DBMON-4188 . Which means that the sleep produced by DBMAsync was always needed for test to function.

I'll take a look at it when I have more time. Probably, tmrw morning. But I am confident that we can move forward with this change.

if sleep is needed regardless of the collection interval, can we just add sleep without setting the collection interval to 0.000001 to minimize the changes?

Adding a bunch of sleeps all over the place seems like a recipe for disaster (how will people know to do this going forward??). It also feels like setting a very low collection interval is hacky / prone to future flakey-ness / not straightforward why we need it. I would just add a run_sync_blocking function or something like that to the DBMAsync job, which has the old behavior and then update the tests to use that. This is a bit weird, because we are adding production code that will only be used by tests, but we do this in plenty of places in the backend and this seems like a fair trade off in order to have reliable tests going forward. We just need to make sure this function is documented well.

github-actions · 2024-06-17T15:36:00Z

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

lu-zhengda · 2024-06-17T16:42:32Z

postgres/tests/test_statements.py

@@ -87,7 +88,7 @@ def test_statement_metrics_multiple_pgss_rows_single_query_signature(
    # don't need samples for this test
    dbm_instance['query_samples'] = {'enabled': False}
    dbm_instance['query_activity'] = {'enabled': False}
-    dbm_instance['query_metrics'] = {'enabled': True, 'run_sync': True, 'incremental_query_metrics': True}
+    dbm_instance['incremental_query_metrics'] = True


this looks wrong. why is incremental_query_metrics moved to the top level?

oh yeah should be
dbm_instance['query_metrics']['incremental_query_metrics'] = True
thx

lu-zhengda · 2024-06-17T16:42:59Z

sqlserver/tests/test_statements.py

@@ -311,10 +314,12 @@ def test_statement_metrics_and_plans(
    # 2) load the test queries into the StatementMetrics state
    # 3) emit the query metrics based on the diff of current and last state
    dd_run_check(check)
+    time.sleep(2)


why do we need sleep here? I thought we need either sleep or a close to zero collection interval but not both

justiniso · 2024-06-17T22:21:53Z

mysql/tests/test_statements.py

@@ -30,15 +30,23 @@
 # depend on a default schema being set on the connection
 DEFAULT_FQ_SUCCESS_QUERY = "SELECT * FROM information_schema.TABLES"

+CLOSE_TO_ZERO_INTERVAL = 0.0000001


Is there a difference between 0.0000001 and 0? Why not just zero?

0 doesn't work cause we divide by 0 when we pass this argument to DBMAsync.
So, in code there is a check that will revert 0 to default_collection_interval.
like https://github.com/DataDog/integrations-core/blob/13b7646c076108d76194c67e80c57515564fd489/sqlserver/datadog_checks/sqlserver/statements.py#L212 .

Got it, thanks!

kozlovb requested review from a team as code owners June 14, 2024 10:10

datadog-agent-integrations-bot bot added changelog/no-changelog integration/sqlserver team/agent-integrations team/database-monitoring-agent labels Jun 14, 2024

kozlovb changed the title ~~Fix tests~~ Fix tests that expect DBMAsync to put thread to sleep in sync mode Jun 14, 2024

datadog-agent-integrations-bot bot added integration/postgres integration/mysql labels Jun 14, 2024

kozlovb added the qa/skip-qa Automatically skip this PR for the next QA label Jun 14, 2024

kozlovb added 15 commits June 17, 2024 10:48

Fix tests

31259a6

Remove pdb

cfa4219

format code

3623ed9

format code more

acd0a81

Fix postgres tests

c2a2f1a

applied linter

b45dc3c

Fix mysql tests

7ba27a0

fix more mysql tests

b7399e9

fix mysql cloud metrics test

a36bffe

set small collection interval differently

07be871

linter applied

4592e49

formatted collection_interval

16d74b8

applied linter

287dd8d

change comment

f57292b

Removed empty line

5d61b1e

kozlovb force-pushed the BorisKozlov-DBMON-4188 branch from 0135858 to 5d61b1e Compare June 17, 2024 10:49

kozlovb commented Jun 17, 2024

View reviewed changes

applied linter

b96447b

lu-zhengda reviewed Jun 17, 2024

View reviewed changes

justiniso reviewed Jun 17, 2024

View reviewed changes

kozlovb and others added 2 commits June 18, 2024 09:21

Fix test settings

74fa659

remove sleep

85aa002

lu-zhengda approved these changes Jun 18, 2024

View reviewed changes

lu-zhengda merged commit b9cef75 into master Jun 18, 2024
42 checks passed

lu-zhengda deleted the BorisKozlov-DBMON-4188 branch June 18, 2024 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests that expect DBMAsync to put thread to sleep in sync mode #17847

Fix tests that expect DBMAsync to put thread to sleep in sync mode #17847

kozlovb commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

kozlovb Jun 17, 2024

lu-zhengda Jun 17, 2024

kozlovb Jun 18, 2024

lu-zhengda Jun 18, 2024

jmeunier28 Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024

lu-zhengda Jun 17, 2024

kozlovb Jun 18, 2024

lu-zhengda Jun 17, 2024

justiniso Jun 17, 2024

kozlovb Jun 18, 2024

justiniso Jun 18, 2024

Fix tests that expect DBMAsync to put thread to sleep in sync mode #17847

Fix tests that expect DBMAsync to put thread to sleep in sync mode #17847

Conversation

kozlovb commented Jun 14, 2024 • edited Loading

What does this PR do?

Review checklist (to be filled by reviewers)

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmeunier28 Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovb commented Jun 14, 2024 •

edited

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

jmeunier28 Jun 18, 2024 •

edited

Loading