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.

Sign up for GitHub

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

Jump to bottom

chore(analytics): track in-app groups with groups metrics #2678

Merged
merged 3 commits into from
Sep 25, 2023
Merged

chore(analytics): track in-app groups with groups metrics #2678

merged 3 commits into from
Sep 25, 2023

Conversation

Cosmin-Parvulescu
Copy link
Contributor

@Cosmin-Parvulescu Cosmin-Parvulescu commented Sep 12, 2023 •
edited
Loading

Description

  • Adapted existing analytic events to take into consideration owning groups
  • Added new analytic events to group platform methods

Related Issues

Testing

image

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@Cosmin-Parvulescu Cosmin-Parvulescu added the enhancement Indicates new feature requests label Sep 12, 2023
@Cosmin-Parvulescu Cosmin-Parvulescu self-assigned this Sep 12, 2023
@Cosmin-Parvulescu Cosmin-Parvulescu marked this pull request as ready for review September 12, 2023 12:12
@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/analytics/track-in-app-groups-with-groups-metrics branch 3 times, most recently from b718b72 to 2e48f90 Compare September 15, 2023 07:26
betimshahini
betimshahini requested changes Sep 20, 2023
View reviewed changes
platform/starbase/src/jsonrpc/methods/publishApp.ts Outdated
dst: { baseUrn: appURN },
},
})
if (ownershipEdges.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have this structure in context from the ownsAppsMiddleware. You should be able to use the data without the additional edges call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this relates to finding out the actual owner URN of the app, which could be the current Identity URN (for own apps) or a Group URN of which the current Identity URN is part of (for all apps). We need to find the owner in order to assess if it's a personal or a group app and set the group analytics entry if it's the latter.

What we don't have in the data is this concept of ownership, I think we only have a list of app urns that are owned either by current Identity URN or a Group URN of which the current Identity URN is part of.

platform/starbase/src/jsonrpc/methods/setAppPlan.ts Outdated
dst: { baseUrn: appURN },
},
})
if (ownershipEdges.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as for publishApp

@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/analytics/track-in-app-groups-with-groups-metrics branch from 2e48f90 to c95b1aa Compare September 21, 2023 08:07
@Cosmin-Parvulescu Cosmin-Parvulescu marked this pull request as draft September 21, 2023 15:22
@Cosmin-Parvulescu
Copy link
Contributor Author

Putting it back on draft as there are some strange things with events posthog side that I need to further investigate.

@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/analytics/track-in-app-groups-with-groups-metrics branch from c95b1aa to 3a65461 Compare September 22, 2023 08:55
@Cosmin-Parvulescu
Copy link
Contributor Author

Reopening PR as events have appeared in the posthog panel:

image

@Cosmin-Parvulescu Cosmin-Parvulescu marked this pull request as ready for review September 22, 2023 08:56
betimshahini
betimshahini requested changes Sep 22, 2023
View reviewed changes
platform/starbase/src/jsonrpc/methods/setAppPlan.ts
)
}

ctx.waitUntil?.(buildAnalyticsEvent())
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine this ctx.waitUntil with the one above. There shouldn't be two here, now that we have an async function that can encapsulate all of the analytics logic.

@Cosmin-Parvulescu Cosmin-Parvulescu force-pushed the chore/analytics/track-in-app-groups-with-groups-metrics branch from 3a65461 to 09eb086 Compare September 23, 2023 01:48
@betimshahini betimshahini merged commit c4f45cd into main Sep 25, 2023
14 checks passed
@betimshahini betimshahini deleted the chore/analytics/track-in-app-groups-with-groups-metrics branch September 25, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betimshahini betimshahini betimshahini approved these changes

Assignees

@Cosmin-Parvulescu Cosmin-Parvulescu

Labels
enhancement Indicates new feature requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore(analytics): track in-app groups with groups metrics
2 participants
@Cosmin-Parvulescu @betimshahini