Skip to content

SNOW-2912540: rewrite telemetry tests to verify the actual wire payload - #4323

Draft
sfc-gh-fpawlowski wants to merge 1 commit into
SNOW-2912540-mitmproxy-capture-fixturefrom
SNOW-2912540-mitmproxy-telemetry-tests
Draft

sfc-gh-fpawlowski wants to merge 1 commit into
SNOW-2912540-mitmproxy-capture-fixturefrom
SNOW-2912540-mitmproxy-telemetry-tests

Conversation

@sfc-gh-fpawlowski

Copy link
Copy Markdown
Collaborator
  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-NNNNNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
    • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support. Follow the link for more information: AST Support Guidelines
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

sfc-gh-fpawlowski commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch 3 times, most recently from 8ccaf7b to 246d926 Compare August 21, 2026 06:53
@codecov-commenter

codecov-commenter commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.04%. Comparing base (81d43c9) to head (de9249b).

Additional details and impacted files
@@                            Coverage Diff                             @@
##           SNOW-2912540-mitmproxy-capture-fixture    #4323      +/-   ##
==========================================================================
- Coverage                                   82.64%   76.04%   -6.61%     
==========================================================================
  Files                                         172      171       -1     
  Lines                                       44811    44716      -95     
  Branches                                     7692     7691       -1     
==========================================================================
- Hits                                        37036    34004    -3032     
- Misses                                       5812     8361    +2549     
- Partials                                     1963     2351     +388     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch 4 times, most recently from 7cf1721 to 3346408 Compare August 21, 2026 09:45
@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch 4 times, most recently from 6451408 to 3f1ca0e Compare August 21, 2026 13:14
@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch from 3f1ca0e to fc2d729 Compare August 21, 2026 13:35
@cursor
cursor Bot force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch from fc2d729 to bdedeaf Compare September 2, 2026 06:45
@sfc-gh-yuwang
sfc-gh-yuwang added this pull request to stack #4349 September 9, 2026 16:52
@sfc-gh-yuwang
sfc-gh-yuwang force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch from 1f5a5ff to 2a95d38 Compare September 9, 2026 18:15
@sfc-gh-yuwang
sfc-gh-yuwang force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch from 2a95d38 to 26b85fe Compare September 9, 2026 18:19
Migrates the 10 single-check TelemetryDataTracker call sites in
tests/integ/test_telemetry.py to a new WireTelemetryChecker that verifies
telemetry against the real /telemetry/send payload captured by the mitmproxy
fixture, instead of the connector's in-process _log_batch buffer (removed on
the Universal Driver, drivers#1104/BD#58).

Session.close() is the only lever that reliably forces an immediate flush on
both drivers (send_log_batch/send_batch are no-ops on the Universal Driver,
where flush is core-owned) -- confirmed against the Rust core source: close()
blocks synchronously on the flush via an in-process PyO3 call, no subprocess
or async race involved. WireTelemetryChecker closes its session as part of
each check, so it needs a dedicated session rather than the shared
module-scoped one; extracted _build_session() out of the `session` fixture in
conftest.py so both it and the (now fully-configured) `mitmproxy_session`
fixture share the same setup/teardown logic. Also fixes a copy-paste bug from
the mitmproxy_session fixture's initial commit: its finally block referenced
an undefined `session` variable and called an unrelated cleanup helper meant
for a different fixture.

test_udf_call_and_invoke, test_sproc_call_and_invoke, and test_udtf_call_and_invoke
stay on the mock-patch-based TelemetryDataTracker (from the prior commit) --
each calls the tracker multiple times against one session, which is
incompatible with close-to-flush. tests/integ/modin/test_telemetry.py's one
remaining case also stays on the mock-patch fix, for the same reason.
@sfc-gh-yuwang
sfc-gh-yuwang force-pushed the SNOW-2912540-mitmproxy-telemetry-tests branch from 26b85fe to de9249b Compare September 9, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants