Skip to content
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

chore: better logging for surveys loaders #1663

Merged
merged 12 commits into from
Jan 22, 2025

Conversation

marandaneto
Copy link
Member

Changes

It won't fix but It'll help figuring out https://posthoghelp.zendesk.com/agent/tickets/21804

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Jan 21, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Jan 22, 2025 6:07pm

Copy link

github-actions bot commented Jan 21, 2025

Size Change: +6.8 kB (+0.21%)

Total Size: 3.27 MB

Filename Size Change
dist/all-external-dependencies.js 209 kB +387 B (+0.19%)
dist/array.full.es5.js 266 kB +758 B (+0.29%)
dist/array.full.js 370 kB +754 B (+0.2%)
dist/array.full.no-external.js 369 kB +754 B (+0.2%)
dist/array.js 182 kB +369 B (+0.2%)
dist/array.no-external.js 180 kB +369 B (+0.21%)
dist/main.js 182 kB +369 B (+0.2%)
dist/module.full.js 370 kB +754 B (+0.2%)
dist/module.full.no-external.js 369 kB +754 B (+0.2%)
dist/module.js 182 kB +369 B (+0.2%)
dist/module.no-external.js 180 kB +369 B (+0.2%)
dist/surveys-preview.js 67.5 kB +406 B (+0.61%)
dist/surveys.js 64.2 kB +387 B (+0.61%)
ℹ️ View Unchanged
Filename Size
dist/customizations.full.js 13.8 kB
dist/dead-clicks-autocapture.js 14.4 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.64 kB
dist/recorder-v2.js 116 kB
dist/recorder.js 116 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

@@ -534,7 +538,11 @@ export const sendSurveyEvent = (
survey: Survey,
posthog?: PostHog
) => {
if (!posthog) return
// TODO: does it need readOnly like dismissedSurveyEvent?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasheriques since you are working on the preview bits, do we need the readOnly here too? is the preview sending dismissed events?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no the preview doesn't send and kind of events

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasheriques so should we remove the checks from the other methods?

@@ -598,7 +598,7 @@ export class PostHog {
this.sessionRecording?.onRemoteConfig(config)
this.autocapture?.onRemoteConfig(config)
this.heatmaps?.onRemoteConfig(config)
this.surveys?.onRemoteConfig(config)
this.surveys.onRemoteConfig(config)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surveys is created on the constructor, never undefined|null

@marandaneto marandaneto added the bump minor Bump minor version when this PR gets merged label Jan 21, 2025
@marandaneto
Copy link
Member Author

@pauldambra just because you're the bundler master and I want to avoid something like https://posthog.slack.com/archives/C07QD3LT8U9/p1737472825108809

Copy link

@ioannisj ioannisj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment, but looks good with my limited know-how :)

src/extensions/surveys.tsx Show resolved Hide resolved
src/extensions/surveys.tsx Show resolved Hide resolved
@pauldambra
Copy link
Member

you can always check #1663 (comment) once it's run (and it will update each time you push) to track the impact :)

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't run it, but looks safe 👍

Copy link
Contributor

@lucasheriques lucasheriques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@marandaneto marandaneto added bump patch Bump patch version when this PR gets merged and removed bump minor Bump minor version when this PR gets merged labels Jan 22, 2025
@lucasheriques lucasheriques merged commit 1a132e5 into main Jan 22, 2025
28 checks passed
@lucasheriques lucasheriques deleted the chore/improve-surveys-logging branch January 22, 2025 18:11
@daibhin
Copy link
Contributor

daibhin commented Jan 23, 2025

Not sure if it's intended but I'm seeing a ton of repeated logs added in this PR. Looks like it's firing once a second
Screenshot 2025-01-23 at 10 22 47

@marandaneto
Copy link
Member Author

Not sure if it's intended but I'm seeing a ton of repeated logs added in this PR. Looks like it's firing once a second Screenshot 2025-01-23 at 10 22 47

good point, here it is #1679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants