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

When DEBUG, include posthog.js with local posthog host #1840

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Oct 12, 2020

Changes

This was initially included in session recording project, but removed in
#1837

Wins of doing it this way:

Downsides:

  • If feature flags are not cleaned up, this may cause drift between
    local checkouts and production.

Checklist

  • All querysets/queries filter by Team (if this PR affects any querysets/queries)
  • Backend tests (if this PR affects the backend)
  • Cypress E2E tests (if this PR affects the front and/or backend)

This was initially included in session recording project, but removed in
#1837

Wins of doing it this way:
- Less noisy data in production.
  - Slack thread by @paolodamico: https://posthog.slack.com/archives/C018RGRS10S/p1602486169005700
- Can develop/dogfood feature flags for development.
  - Used in session recording - can "merge" half-complete solutions that
    are not yet ready for release
- Session recording can be done in localhost now :)

Downsides:
- If feature flags are not cleaned up, this may cause drift between
  local checkouts and production.
@timgl timgl temporarily deployed to posthog-debug-use-local-kcjmi8 October 12, 2020 13:09 Inactive
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

In general terms my only concern with this is that this will cause users running debug instances to get events from their PostHog app usage as events from their own app, which can be a bit disconcerting.

As I'm working on #1755 I ran into the same issue that prompted you to consider this change, my solution (unpushed) was this (though I wasn't considering the Cypress case you mentioned). Basically we this we get feature flags without getting noisy data from debug instances.

<script>
    !function(t,e){var o,n,p,r;e.__SV||(window.posthog=e,e._i=[],e.init=function(i,s,a){function g(t,e){var o=e.split(".");2==o.length&&(t=t[o[0]],e=o[1]),t[e]=function(){t.push([e].concat(Array.prototype.slice.call(arguments,0)))}}(p=t.createElement("script")).type="text/javascript",p.async=!0,p.src=s.api_host+"/static/array.js",(r=t.getElementsByTagName("script")[0]).parentNode.insertBefore(p,r);var u=e;for(void 0!==a?u=e[a]=[]:a="posthog",u.people=u.people||[],u.toString=function(t){var e="posthog";return"posthog"!==a&&(e+="."+a),t||(e+=" (stub)"),e},u.people.toString=function(){return u.toString(1)+".people (stub)"},o="capture identify alias people.set people.set_once set_config register register_once unregister opt_out_capturing has_opted_out_capturing opt_in_capturing reset isFeatureEnabled onFeatureFlags".split(" "),n=0;n<o.length;n++)g(u,o[n]);e._i.push([i,s,a])},e.__SV=1)}(document,window.posthog||[]);
    posthog.init('sTMFPsFhdP1Ssg', {api_host: 'https://app.posthog.com'{% if debug %}, autocapture: false, capture_pageview: false{% endif %}})
</script>

posthog/utils.py Show resolved Hide resolved
posthog/utils.py Outdated Show resolved Hide resolved
@macobo
Copy link
Contributor Author

macobo commented Oct 12, 2020

Thanks for the feedback @paolodamico

In general terms my only concern with this is that this will cause users running debug instances to get events from their PostHog app usage as events from their own app, which can be a bit disconcerting.

@paolodamico Isn't that desirable? My assumption is DEBUG = development environment, correct me if I'm wrong. If not, maybe worth adding another env variable?

Basically we this we get feature flags without getting noisy data from debug instances.

This has significant downsides - having to manage dev feature flags from prod makes dogfooding them harder. See wins 2-4

@macobo macobo temporarily deployed to posthog-debug-use-local-kcjmi8 October 13, 2020 07:08 Inactive
@Twixes
Copy link
Collaborator

Twixes commented Oct 13, 2020

I like this a lot, I think we can assume that DEBUG users are likely to actually want these events for testing PostHog on actual data (I want em!). And for those who don't want them, there's env var OPT_OUT_CAPTURE.

@yakkomajuri
Copy link
Contributor

+1 for this.

@paolodamico
Copy link
Contributor

Well my only outstanding concern is with users that run the app with DEBUG=1 because they want to test something / use quickly, but I do agree that DEBUG should be intended for development and therefore this should be the best approach right now.

@macobo macobo merged commit 679d9ec into master Oct 13, 2020
@macobo macobo deleted the debug-use-local-instance branch October 13, 2020 11:56
timgl added a commit that referenced this pull request Oct 13, 2020
@paolodamico
Copy link
Contributor

I think this PR may have introduced a minor typo that breaks feature flags and all tracking. See below screenshot of the current code on PostHog cloud, notice the &#x27; in the api_host.

image

@Twixes
Copy link
Collaborator

Twixes commented Oct 14, 2020

macobo added a commit that referenced this pull request Oct 14, 2020
This broke tracking

See also #1840 (comment)
@macobo
Copy link
Contributor Author

macobo commented Oct 14, 2020

#1868

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.

None yet

5 participants