Skip to content

refactor(telemetry): route execution events to GTM only (MAR-282)#12717

Merged
benceruleanlu merged 1 commit into
mainfrom
canonical-execution-fix
Jun 9, 2026
Merged

refactor(telemetry): route execution events to GTM only (MAR-282)#12717
benceruleanlu merged 1 commit into
mainfrom
canonical-execution-fix

Conversation

@stevenltran

Copy link
Copy Markdown
Contributor

Summary

Client-side execution events (execution_start / execution_success / execution_error) are now emitted only by the GTM provider, removing the redundant Mixpanel and PostHog emissions that duplicated the server-side PostHog execution pipeline.

Changes

  • Removed trackWorkflowExecution, trackExecutionError, and trackExecutionSuccess from MixpanelTelemetryProvider and PostHogTelemetryProvider, along with the now-unused lastTriggerSource field and related type imports.
  • Kept these methods on GtmTelemetryProvider. The TelemetryProvider interface declares them optional and TelemetryRegistry dispatches via optional chaining, so callers are unchanged and Mixpanel/PostHog simply receive nothing for these events.
  • Added GTM unit tests for execution_start and execution_success (alongside the existing execution_error test) to pin the remaining client-side path.

Review Focus

  • Execution telemetry on the client now flows exclusively to GTM; PostHog execution data is expected to come solely from the server side, so there should be no double-counting.
  • The server-side PostHog execution pipeline is out of scope for this frontend change — this PR only stops the client from emitting duplicate execution events.

Reference: MAR-282
Prior context: Comfy-Org PR #3423.

Remove client-side execution_start/success/error tracking from the
Mixpanel and PostHog providers. These events are now emitted solely by
the GTM provider, avoiding duplication with the server-side PostHog
execution pipeline.

Add GTM tests for execution_start and execution_success to pin the
remaining client-side path.
@stevenltran stevenltran requested review from a team, comfyui-wiki and dante01yoon June 8, 2026 20:07
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

🎨 Storybook: ✅ Built — View Storybook

Details

⏰ Completed at: 06/08/2026, 08:09:30 PM UTC

Links

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

🎭 Playwright: ✅ 1664 passed, 0 failed · 2 flaky

📊 Browser Reports
  • chromium: View Report (✅ 1645 / ❌ 0 / ⚠️ 1 / ⏭️ 5)
  • chromium-2x: View Report (✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0)
  • chromium-0.5x: View Report (✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0)
  • mobile-chrome: View Report (✅ 16 / ❌ 0 / ⚠️ 1 / ⏭️ 0)

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 8, 2026
@stevenltran

Copy link
Copy Markdown
Contributor Author

@dante01yoon and @comfyui-wiki

it looks like you guys most recently made changes to these files - do you mind taking a look and merging this?

funneling the client side execution to only gtm now not mixpanel and posthog
server side execution should still be in tact to posthog (main source of truth)

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@             Coverage Diff             @@
##             main   #12717       +/-   ##
===========================================
- Coverage   75.23%   61.35%   -13.89%     
===========================================
  Files        1552     1449      -103     
  Lines       89218    75005    -14213     
  Branches    24528    21169     -3359     
===========================================
- Hits        67126    46021    -21105     
- Misses      21340    28630     +7290     
+ Partials      752      354      -398     
Flag Coverage Δ
e2e ?
unit 61.35% <ø> (+0.80%) ⬆️

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

Files with missing lines Coverage Δ
...metry/providers/cloud/MixpanelTelemetryProvider.ts 80.36% <ø> (-1.25%) ⬇️
...emetry/providers/cloud/PostHogTelemetryProvider.ts 59.03% <ø> (+11.69%) ⬆️

... and 1185 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stevenltran stevenltran requested a review from benceruleanlu June 8, 2026 20:35
@benceruleanlu benceruleanlu added this pull request to the merge queue Jun 9, 2026
Merged via the queue into main with commit 8972d27 Jun 9, 2026
57 checks passed
@benceruleanlu benceruleanlu deleted the canonical-execution-fix branch June 9, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants