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

refactor(group-analytics): Add project field to group type #25600

Merged
merged 26 commits into from
Oct 31, 2024

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 15, 2024

Problem

Step 6 of the environments implementation plan.

Changes

Added nullable field project to the GroupTypeMapping model. We now insert it in ingestion.

Backfill for existing group types will come in a separate PR.

How did you test this code?

All existing tests should continue to pass.

@Twixes Twixes requested a review from oliverb123 October 15, 2024 14:45
@oliverb123
Copy link
Contributor

oliverb123 commented Oct 15, 2024

Hey @Twixes, have you checked how this interacts with property definitions? We rely on group-type mapping for certain property types. Not specific to this PR possibly, more the overall change

@oliverb123
Copy link
Contributor

oliverb123 commented Oct 16, 2024

Just watched your brown-bag (nice one btw) and I feel like, if I understood correctly, this should remain at the "team (environment)" level? My gut says the set of group types is a property of the event data, rather than something users configure (hence why we create it on the fly during ingest rather than having users manually create it with some tagging rules, like an insight or dashboard).

I don't know that this is the right model for what a group type is, or whether we should make users declare them up front, but it is what it is today.

@Twixes Twixes force-pushed the group-type-project branch from 2d0726d to bc2a8d9 Compare October 28, 2024 21:05
@Twixes
Copy link
Member Author

Twixes commented Oct 29, 2024

Coming back to this now that some other PRs are in. To answer both your questions @oliverb123:

So, the concept behind environments is that data coming in/out is environment-specific, but the shape of that data lives at the project level – as it's the same instrumentation sending that data, only running in different environments. The same applies with event and property definitions, they define the shape (and users can configure them).
This PR here should not affect event/property definitions in any way. They're getting the project field too, but that'll be a separate PR.

@Twixes Twixes merged commit 12ff477 into master Oct 31, 2024
89 checks passed
@Twixes Twixes deleted the group-type-project branch October 31, 2024 23:12
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.

2 participants