SNOW-1763096: Fix async telemetry support #2590
Conversation
sfc-gh-turbaszek
force-pushed
the
turbaszek-SNOW-1763096-fix-aio-telemetry
branch
from
5dd03c6 to
4d49b11
Compare
| return isinstance(connection, AsyncSnowflakeConnection) | ||
| except ImportError: | ||
| return False | ||
|
|
There was a problem hiding this comment.
Removing this because telemetry about aio vs sync is included in application_name param in telemetry payload.
There was a problem hiding this comment.
Do we have tests that assure that information is in application_name in telemetry payload? Such tests would naturally replace the deleted ones.
There was a problem hiding this comment.
We do have test_generate_telemetry_data which validates behavior of from_telemetry_data_dict which is used to build telemetry data everywhere.
There was a problem hiding this comment.
Okay, I was thinking more of a test that could prevent some future unification of App_name between async driver and the sync one - due to lack of knowledge/ comment/ tests indicating that it is indeed the expected behaviour.
Its a nit tho.
sfc-gh-fpawlowski
left a comment
sfc-gh-fpawlowski
left a comment
There was a problem hiding this comment.
Left one question regarding preserving the expected telemetry behaviour in tests - to avoid accidental changes of that in the future.
| return isinstance(connection, AsyncSnowflakeConnection) | ||
| except ImportError: | ||
| return False | ||
|
|
There was a problem hiding this comment.
Do we have tests that assure that information is in application_name in telemetry payload? Such tests would naturally replace the deleted ones.
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes #NNNN
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.
(Optional) PR for stored-proc connector: