SNOW-2171791: Add platform telemetry #2387

sfc-gh-sshetkar · 2025-07-04T00:04:29Z

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.

SNOW-2171791
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am adding a new telemetry message
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency
Please describe how your code solves the related issue.

This PR adds platform detection mechanisms and appends them to the CLIENT_ENVIRONMENT part of the base_auth_data(). This then gets sent to the /login-request endpoint which serializes and stores this data for future use in the SessionDPO.

To avoid bottlenecking the authentication flow, we have parallelized all platform detection I/O calls with a default timeout of 200 milliseconds. Overall testing has found that these tests take on the order of 1-3 milliseconds with more details provided in the design doc. We have also exposed a user parameter so that they can configure how long of a timeout they are ok with.

Design Doc

(Optional) PR for stored-proc connector:

…nd GCE cloud run

…and I don't want it to block the initialization

…ion checks because it throws internal exceptions now instead of requests library exceptions

…ake resource

…a background thread and switched to executing the checks in parallel for the internal metadata network calls

…alse values

… will get sent to login-request and eventually persisted in the SessionDPO

github-actions · 2025-07-04T00:04:41Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

… this file

sfc-gh-sshetkar · 2025-07-08T22:16:52Z

I have read the CLA Document and I hereby sign the CLA

…meout, set new default timeout to 0.2s, added new integration test to test user parameter, updated unit tests and added one unit test to test configuring the timeout

…s of azure functions or azure VM for speed. It does extra work but since we parallelize we would rather not have two I/O options forced to be serialized one after another.

…ave to make sure the function does not affect main driver functionality

… unnecessary mock

… wif. slight adjustments to platform detection for URL fixes/header fixes that don't affect logic but make checks in the the csphelper more consistent

…ronment variables and can be independent of the platfrom it is running on. This helps with the GitHub actions test since the test is actually run on GitHub actions.

…ailures

… additively adds environments for every fake environment

…each test as well as an additional safety measure

sfc-gh-pmansour

Thanks Sid!

src/snowflake/connector/auth/_auth.py

src/snowflake/connector/connection.py

src/snowflake/connector/platform_detection.py

test/unit/test_detect_platforms.py

src/snowflake/connector/platform_detection.py

test/unit/test_detect_platforms.py

sfc-gh-pmansour · 2025-08-01T03:13:06Z

test/unit/test_detect_platforms.py

+    def test_custom_timeout_parameter(
+        self, mock_requests, mock_boto3, mock_imds_fetcher
+    ):
+        mock_imds_instance = Mock()
+        mock_imds_instance._get_request.side_effect = Exception("No IMDS")
+        mock_imds_fetcher.return_value = mock_imds_instance
+
+        mock_boto3.client.return_value.get_caller_identity.side_effect = Exception(
+            "No AWS"
+        )
+
+        mock_requests.get.side_effect = requests.RequestException("No metadata")
+        mock_requests.RequestException = requests.RequestException
+        mock_requests.Timeout = requests.Timeout
+
+        result = detect_platforms(timeout=5.0)
+
+        assert "is_aws_lambda" in result
+        assert "is_github_action" in result


I do think there's value in having a test that ensures the timeout is respected and the entire method finishes within that (+ some minor error margin).

Currently, if you have a bug that does everything in serial (rather than parallel), or that forgets to pass the timeout, or that passes the wrong timeout variable, you have no way to detect it. In fact, I believe you previously had such a bug (with the azure managed identity), and may currently have another one (with the 10 executors rather than 11), but there are no tests to catch it.

sfc-gh-pmansour · 2025-08-01T03:15:17Z

test/unit/test_detect_platforms.py

+            raise requests.HTTPError(f"HTTP {self.status_code} Error")
+
+
+class TestDetectPlatforms:


Generally it's good to keep tests focused and test behaviors, not methods, with each test verifying a specific behavior. Otherwise, if you introduce a bug, you'll have to wade through 100 failing tests and try to figure out what went wrong, rather than just 1.

Anyway, I think the negative test coverage you have here is probably sufficient for now. Thanks!

…ection code

…et a field for the type of exception to be thrown. Then, the platform detection tests has a fixture to set this field to RequestException for tests

…ent most of them. Updated GCE cloud run service/job to inherit GCEFakeMetadaService to get its functionality as well

…t instead of reusing the FakeAwsEnvironment class

…with fake issuer and instead just use a flag to raise a timeout

…t exist. Don't want to fail everything there due to Attribute Error. Updated test to include a parallel timing test

… IMDS fetcher could take in timeouts

sfc-gh-pmansour

LGTM, thanks for all your patience!

Would be good to get @sfc-gh-mmishchenko's LGTM too

sfc-gh-sshetkar added 12 commits June 23, 2025 22:39

added telemetry emission for AWS Lambda, AWS EC2, Azure VM, GCE VM, a…

3e3d732

…nd GCE cloud run

added github action platform check

0d6683b

linter fixes

f7879bd

added checks for managed identity

2e5168f

put telemetry on a separate thread since it makes some network calls …

a8ad08d

…and I don't want it to block the initialization

added check for aws identity and refactored ec2 check to use AWS library

ce185da

updated name for aws arn identity check function

b1734ec

added gcp identity checks and updated AWS checks to do generic except…

99f4bf8

…ion checks because it throws internal exceptions now instead of requests library exceptions

updated azure managed identity check to go against the default snowfl…

c66c054

…ake resource

switched to 0.5 second timeout because eventually it won't be run on …

d62e955

…a background thread and switched to executing the checks in parallel for the internal metadata network calls

changed to return list of signals instead of dictionary of true and f…

44ac14c

…alse values

switched to detect platforms by adding it to the base_auth data which…

d163d32

… will get sent to login-request and eventually persisted in the SessionDPO

sfc-gh-sshetkar added 7 commits July 4, 2025 00:08

changed gcp identity check to be 0.5 seconds for timeout

69c637e

Merge branch 'main' into sshetkar-SNOW-2171791-add-platform-telemetry

87668c6

Merge branch 'main' into sshetkar-SNOW-2171791-add-platform-telemetry

fb4ccf7

added unit tests for detecting platforms

7a527ab

refactored platform detection to it's own file and updates unit tests

fc611be

updated unit tests

48e0541

mocked detect_platform in connection tests to not affect the tests in…

2394414

… this file

sfc-gh-sshetkar added NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector labels Jul 8, 2025

sfc-gh-sshetkar added 7 commits July 9, 2025 16:01

Merge branch 'main' into sshetkar-SNOW-2171791-add-platform-telemetry

0963b72

changed to 200 millisecond timeout

4467c00

skip test_auth_mfa on windows since it runs inconsistently on that OS

bfb4b67

added parameter for timeout that user can configure

f0dd5c7

added None type to detect_platforms so it gives correct type hinting

0d347d4

added timeout signals, added parameter for user to be able control ti…

bd7082a

…meout, set new default timeout to 0.2s, added new integration test to test user parameter, updated unit tests and added one unit test to test configuring the timeout

added getter and setter for new platform detection timeout field

bff44f1

sfc-gh-sshetkar added 14 commits July 28, 2025 03:42

chanaged azure managed identity check to not depend on previous check…

06f94cb

…s of azure functions or azure VM for speed. It does extra work but since we parallelize we would rather not have two I/O options forced to be serialized one after another.

add try except clause on the detect_platform so that no exceptions le…

9710ce2

…ave to make sure the function does not affect main driver functionality

clear cache after every test run. used actual response class to avoid…

7917c0d

… unnecessary mock

updated tests and csphelpers to use consistent mock enviornments with…

971a0bd

… wif. slight adjustments to platform detection for URL fixes/header fixes that don't affect logic but make checks in the the csphelper more consistent

added parameterized test for testing valid aws arn identities

8e79c1d

added negative tests

527efd9

Merge branch 'main' into sshetkar-SNOW-2171791-add-platform-telemetry

6b7a1a2

added yield so cache clearing happens after every test

a2c4049

updated os dictionary patching so it shouldn't use the actual OS envi…

c27b8ec

…ronment variables and can be independent of the platfrom it is running on. This helps with the GitHub actions test since the test is actually run on GitHub actions.

updated parameter name injected in connection integration test

787ffe7

removed caching off the function to test if that is what is causing f…

c082f40

…ailures

patched os dictionary so that is clears it before every test and then…

fe14cd8

… additively adds environments for every fake environment

added caching back in to see if it runs on github actions tests

3e16e1c

make test run serially to avoid caching issue and clear cache before …

e7995a6

…each test as well as an additional safety measure

sfc-gh-pmansour approved these changes Aug 1, 2025

View reviewed changes

sfc-gh-sshetkar added 9 commits August 1, 2025 18:37

addressed comments related to fixed comments, optimizing platform det…

a8085f7

…ection code

updated name for has_azure_managed_identity in the signal response

f4dcfe6

got rid of BrokenMetadataService and changed to have the base class s…

62c2fad

…et a field for the type of exception to be thrown. Then, the platform detection tests has a fixture to set this field to RequestException for tests

updated github to not use fakemetadataservice since it doesn't implem…

5918f5b

…ent most of them. Updated GCE cloud run service/job to inherit GCEFakeMetadaService to get its functionality as well

updated to separate out AWS lambda as another fake class and used tha…

c7a238c

…t instead of reusing the FakeAwsEnvironment class

updated fake azure vm with no metadata endpoint to not use weird way …

2134261

…with fake issuer and instead just use a flag to raise a timeout

Merge branch 'main' into sshetkar-SNOW-2171791-add-platform-telemetry

f3319c5

added check in gce vm check to not throw an exception if headers don'…

bdacd5c

…t exist. Don't want to fail everything there due to Attribute Error. Updated test to include a parallel timing test

mocked the timeout parallel test properly to match what values of the…

6e3a969

… IMDS fetcher could take in timeouts

sfc-gh-pmansour approved these changes Aug 1, 2025

View reviewed changes

forgot to unpack args parameter on __exit__ for FakeAwsLambdaEnvironment

440dc28

sfc-gh-mmishchenko approved these changes Aug 4, 2025

View reviewed changes

sfc-gh-sshetkar merged commit 0f776c5 into main Aug 4, 2025
91 of 97 checks passed

sfc-gh-sshetkar deleted the sshetkar-SNOW-2171791-add-platform-telemetry branch August 4, 2025 18:26

github-actions bot locked and limited conversation to collaborators Aug 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SNOW-2171791: Add platform telemetry #2387

SNOW-2171791: Add platform telemetry #2387

Uh oh!

sfc-gh-sshetkar commented Jul 4, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 4, 2025 •

edited

Loading

Uh oh!

sfc-gh-sshetkar commented Jul 8, 2025

Uh oh!

sfc-gh-pmansour left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfc-gh-pmansour Aug 1, 2025

Uh oh!

sfc-gh-pmansour Aug 1, 2025

Uh oh!

sfc-gh-pmansour left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		raise requests.HTTPError(f"HTTP {self.status_code} Error")


		class TestDetectPlatforms:

SNOW-2171791: Add platform telemetry #2387

SNOW-2171791: Add platform telemetry #2387

Uh oh!

Conversation

sfc-gh-sshetkar commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfc-gh-sshetkar commented Jul 8, 2025

Uh oh!

sfc-gh-pmansour left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfc-gh-pmansour Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-pmansour Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-pmansour left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sfc-gh-sshetkar commented Jul 4, 2025 •

edited

Loading

github-actions bot commented Jul 4, 2025 •

edited

Loading