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

wip: support test_ api key #4206

Merged
merged 36 commits into from
May 21, 2021
Merged

wip: support test_ api key #4206

merged 36 commits into from
May 21, 2021

Conversation

buwilliams
Copy link
Contributor

@buwilliams buwilliams commented May 4, 2021

Changes

  • Support for test_[apiKey] which signifies local, test, and staging environments
  • Allows for filtering all data by drawing a line between Production and everything other env
  • test_[apiKey] sets $environment to test
  • Users may also pass in this property to override or simply set the value for $environment
  • Added environment toggle to top navigation and put it behind a feature flag

See #3149 for more information

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious

@buwilliams buwilliams self-assigned this May 4, 2021
@timgl timgl temporarily deployed to posthog-pr-4206 May 4, 2021 14:53 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 4, 2021 15:33 Inactive
posthog/api/capture.py Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-pr-4206 May 4, 2021 18:19 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 4, 2021 19:38 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 4, 2021 20:05 Inactive
@buwilliams
Copy link
Contributor Author

Anyone know what this check is failing? It's a python issue that doesn't make sense to me.

@timgl timgl temporarily deployed to posthog-pr-4206 May 5, 2021 03:07 Inactive
@paolodamico
Copy link
Contributor

Re @buwilliams,

Anyone know what this check is failing? It's a python issue that doesn't make sense to me.

The check that was failing indicated that there was a typing problem. Python has only limited support for typing, introduced fairly recently (see more details). One of the limitations is that typing is not actually checked when coding (some IDEs might support this) or at runtime. Instead, we use https://github.com/python/mypy to check typing. You can run it yourself locally by doing DEBUG=1 mypy .

See 87018fc for details on how the specific typing issue was fixed.

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.

Added some comments. Also proposing a different UI for the toggle (feel free to challenge / iterate).

  1. Also worth noting, what happens when you change the filter? Ideally we would reload the graph, table you're viewing.
  2. Finally, we should also remove the test filters toggle from the UI if the feature flag is enabled.

frontend/src/scenes/userLogic.tsx Outdated Show resolved Hide resolved
frontend/src/lib/components/PropertyKeyInfo.tsx Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-pr-4206 May 5, 2021 03:39 Inactive
@paolodamico
Copy link
Contributor

Here's my proposed UI for this. Feedback @corywatilo too?

image

Outstanding

Please note I think we should also add a global indicator to all pages to be more explicit when you're on test data mode. Something like this,

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Hey! So, I posted the original env separation issue, and I think this implementation may be too narrow. I think it's quite possible we'll want to support more such environments than with just a boolean switch (e.g. selector a la Sentry), so it'd be more flexible if instead of $test_environment: true/false we used $environment: 'production'/'development'.
Aaand, that still does not address a key issue that motivates this feature request: users want to get rid of environments with junk data, but that's not possible - events, once ingested, they history forever, because we don't delete CH events yet. With projects that's kind of solved because the data is just inaccessible once the project is deleted, thanks to using team_id for all filtering.

@buwilliams
Copy link
Contributor Author

@Twixes Why does removing the data matter? Filtering removes unwanted results so the effect is the same from a UI perspective. Additionally, if a flag such as test_environment is present, deleting that data becomes trivial. In either case, removing the data seems like a follow-up issue. One that allows users to purge data based on a filter.

The reason for having boolean rather than by environment was to aid UX since we can think about production and all others and two distinct things and present a simple UX for it. Otherwise, we'd need to create an additional table where users would be to specify their environments so that we could group them appropriately. Seems to me, if an advanced user wanted to track per environment, they could add this as a regular event property. Something like: environment=production

@buwilliams
Copy link
Contributor Author

buwilliams commented May 5, 2021

I've changed the Boolean to a string and renamed it to $environment. Currently, it supports: "production" and "test". Also, I've moved it into a hidden property. Push coming soon.

@Twixes
Copy link
Collaborator

Twixes commented May 5, 2021

Yep, okay, actually removing the data from our disks doesn't matter (that much), but removing it from the user's point of view (meaning just making it completely inaccessible) matters. So it's enough to just always and everywhere filter based on this property. And the environments for a project can be just a Postgres array of strings on the Team model for now.
BTW if we introduce this, it'd be better as an actual Postgres/ClickHouse column on the events table – getting the property from JSON is significantly more expensive. Deployments & Infrastructure would be of help.

@paolodamico
Copy link
Contributor

I agree with @buwilliams, I think multiple environments could be a more advanced use and to consider for later.

In terms of data deletion, I'm torn but leaning towards this is the right approach. Deleting data is a separate thing we need to tackle.

@buwilliams perhaps it's useful to add the context on why we decided against separate projects for this ?

@buwilliams
Copy link
Contributor Author

I'm blocked until M1. What I thought was a bug in my code was not. Events are not ingested because we process events by sending them to a job queue on our plugin server. I was misguided because /demo inserts history for us which appeared as though I was receiving events. Our plugin server relies on Clickhouse which isn't support on M1 as most of you know. Maybe someone knows a way around this but I haven't been able to find it.

@mariusandra
Copy link
Collaborator

mariusandra commented May 9, 2021

To clarify one point, the plugin server definitely does not rely on clickhouse, as we also use the same plugin server for the OSS / non-clickhouse installations. Without explicitly setting KAFKA_ENABLED to true, the plugin server uses the standard postgres ingestion.

@timgl timgl temporarily deployed to posthog-pr-4206 May 10, 2021 12:45 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 10, 2021 12:52 Inactive
@buwilliams
Copy link
Contributor Author

Solved the issue with the plugin-server by running yarn build; WORKER_CONCURRENCY=2 yarn start:dist and created an issue #370. This is ready for merge once all the checks pass, we can continue to improve it in another PR.

@timgl timgl temporarily deployed to posthog-pr-4206 May 10, 2021 13:26 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 11, 2021 15:47 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 11, 2021 18:08 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 13, 2021 19:08 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 13, 2021 19:11 Inactive
@timgl timgl temporarily deployed to posthog-pr-4206 May 13, 2021 19:13 Inactive
@buwilliams buwilliams merged commit b15232a into master May 21, 2021
@buwilliams buwilliams deleted the multiple-environments branch May 21, 2021 15:34
mariusandra added a commit that referenced this pull request Oct 5, 2021
paolodamico added a commit that referenced this pull request Oct 11, 2021
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.

6 participants