-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
feat(telemetry): Adding Scarf based telemetry to Superset #26011
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26011 +/- ##
==========================================
+ Coverage 69.10% 69.41% +0.31%
==========================================
Files 1940 1946 +6
Lines 75869 78721 +2852
Branches 8445 8935 +490
==========================================
+ Hits 52427 54642 +2215
- Misses 21267 21772 +505
- Partials 2175 2307 +132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fd0dfbe
to
a8e3bf9
Compare
The John ❤️ JS commit cracked me up! 🤣 |
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.
LGTM with a minor nit, looking forward to this 👍
superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/TelemetryPixel/TelemetryPixel.test.tsx
Outdated
Show resolved
Hide resolved
… comes through as a string or a boolean.
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.
LGTM
return process.env.SCARF_ANALYTICS === 'false' || | ||
process.env.SCARF_ANALYTICS === 'false' ? null : ( |
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.
process.env.SCARF_ANALYTICS === 'false' ||
process.env.SCARF_ANALYTICS === 'false'
Just noticed this is duplicated.
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.
Yeesh... I thought I nabbed that. Will open a PR to clean up... Especially if this does not have the intended effect now. Surprised some linting rule doesn't spot this kind of thing.
…ache#26011)" This reverts commit 8437a23.
…ache#26011)" This reverts commit 8437a23.
…ache#26011)" This reverts commit 8437a23.
Co-authored-by: Michael S. Molina <[email protected]>
Co-authored-by: Michael S. Molina <[email protected]>
SUMMARY
This PR does a handful of things:
TelemetryPixel
componentENABLE_TELEMETRY
feature flagno-referrer-when-downgrade
TypeError: Cannot redefine property: window
errorTODO:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
RTL tests for the new component with FF on and off should pass. Run it, and have any PMC member check the Scarf data for proper capture.
ADDITIONAL INFORMATION