-
Notifications
You must be signed in to change notification settings - Fork 113
Testing for telemetry #616
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
base: telemetry
Are you sure you want to change the base?
Conversation
Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>
|
||
|
||
class TestTelemetryRaceConditions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add these in a separate file? @jprakash-db what's the sop in python?
tests/unit/test_telemetry.py
Outdated
thread.join() | ||
|
||
assert len(operation_results) == 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should have some tests where there are concurrently threads executing queries and the telemetry is being captured correctly, is that possible to do?
tests/unit/test_telemetry.py
Outdated
assert isinstance(client, NoopTelemetryClient) | ||
|
||
def test_telemetry_client_factory_mixed_concurrent_operations(self, race_condition_setup): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to have separate tests for this i.e. concurrent_init, concurrent_get, concurrent_flush etc? can we not simply have a larger thread pool and just do all of these in a single test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping separate tests helps in finding which operation specifically is causing the race condition. The test_telemetry_client_factory_mixed_concurrent_operations
tests the mixed-operation scenario which is more realistic. So both scenarios (focused on a particular operation and the mixed one) are covered.
Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>
Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>
What type of PR is this?
Description
Added multithread tests for race conditions
How is this tested?
Related Tickets & Documents
PECOBLR-584