feat(onboarding): add logging #75679

ameliahsu · 2024-08-06T20:27:14Z

added logging to monitor the impact of new messaging integration onboarding button

also converted setup button from a class component to a functional component which can replace the existing "Set up Slack Now" button

codecov · 2024-08-06T20:37:02Z

Codecov Report

Attention: Patch coverage is 64.51613% with 11 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (2f8231f) to head (e4c6996).
Report is 111 commits behind head on master.

Files	Patch %	Lines
...ts/rules/issue/setupMessagingIntegrationButton.tsx	72.72%	6 Missing ⚠️
static/app/views/alerts/rules/issue/index.tsx	0.00%	3 Missing ⚠️
...app/views/alerts/rules/issue/addIntegrationRow.tsx	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #75679      +/-   ##
==========================================
- Coverage   78.25%   78.23%   -0.02%     
==========================================
  Files        6826     6833       +7     
  Lines      303279   303541     +262     
  Branches    52188    52240      +52     
==========================================
+ Hits       237323   237479     +156     
- Misses      59579    59686     +107     
+ Partials     6377     6376       -1

Files	Coverage Δ
static/app/utils/analytics/integrations/index.ts	`100.00% <ø> (ø)`
.../app/utils/analytics/onboardingAnalyticsEvents.tsx	`100.00% <ø> (ø)`
...s/alerts/rules/issue/messagingIntegrationModal.tsx	`87.50% <ø> (ø)`
...alerts/rules/issue/setupAlertIntegrationButton.tsx	`81.25% <ø> (+3.83%)`	⬆️
...ttings/organizationIntegrations/addIntegration.tsx	`69.38% <ø> (ø)`
...ngs/organizationIntegrations/integrationButton.tsx	`66.66% <100.00%> (+3.03%)`	⬆️
...gs/organizationIntegrations/integrationContext.tsx	`100.00% <ø> (ø)`
...ganizationIntegrations/integrationDetailedView.tsx	`65.60% <ø> (-0.28%)`	⬇️
...app/views/alerts/rules/issue/addIntegrationRow.tsx	`71.42% <60.00%> (-5.05%)`	⬇️
static/app/views/alerts/rules/issue/index.tsx	`76.38% <0.00%> (-0.57%)`	⬇️
... and 1 more

... and 188 files with indirect coverage changes

malwilley · 2024-08-06T21:56:55Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+ isLoading,
+ isError,
+ refetch,
+ } = useApiQuery<{}>(


Instead of {}, pass the type you expect in response here. That will make project that type and you won't need the as below

malwilley · 2024-08-06T22:00:13Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+ {staleTime: Infinity}
+ );
+
+ const detailedProject = project as Project & {


You can define this type outside the component as:

interface ProjectWithAlertIntegrationInfo extends Project { hasAlertIntegrationInstalled: boolean; }

Using interface and extends is better when adding properties to an existing type since it is more performant and it works more how you would expect it to (type intersection can result in some confusing types in some situations)

malwilley · 2024-08-06T22:02:40Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+ isError,
+ refetch,
+ } = useApiQuery<{}>(
+ [`/projects/${organization.slug}/${projectSlug}/?expand=hasAlertIntegration`],


Not that big of a deal, but you can format it like this instead:

[`/projects/${organization.slug}/${projectSlug}/`, {query: { expand: 'hasAlertIntegration' }}],

malwilley · 2024-08-06T22:03:38Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+ deps => (
+ <MessagingIntegrationModal
+ {...deps}
+ headerContent={<h1>Connect with a messaging tool</h1>}


Static strings should be wrapped in t('...') so that they can be translated

malwilley · 2024-08-06T22:11:33Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.spec.tsx

+ hasAlertIntegrationInstalled: true,
+ },
+ });
+ const {container} = render(getComponent());


Instead of checking the container, I'd check that no button exists:

expect(screen.queryByRole('button')).not.toBeInTheDocument()

malwilley · 2024-08-06T22:12:55Z

static/app/views/alerts/rules/issue/messagingIntegrationModal.tsx

@@ -32,6 +33,13 @@ function MessagingIntegrationModal({
 project,
 onAddIntegration,
 }: Props) {
+ useEffect(() => {
+ trackAnalytics('onboarding.messaging_integration_modal_rendered', {


Usually we don't track analytics when something is rendered - it can result in a lot of excess analytic events if done wrong. Generally it's better to record the analytic on an action instead. I'd recommend firing this event when the button is clicked to open this modal.

If you haven't seen it yet, you can add them on the button very easily: https://develop.sentry.dev/development/analytics/#button-click-analytics

+1 to the button clicks - just wanted to also thank you for that link! i've been hoping to find something like that! 🎉

@malwilley @saponifi3d We need to track how many users are actually shown this setup button so that we can compare this number to how many people actually click the button and install an integration, I'm not sure if there's a better way than tracking when this button is rendered.

🤔 maybe trigger the onboarding.messaging_integration_modal_rendered onClick of the modal?

That way you could visualize the usage through:

Page view

Modal Open

Setup Integration

and we'd be able to understand where a user might get stuck trying to setup the integration.

Ah I think I misread this earlier-- I can definitely change when onboarding.messaging_integration_modal_rendered is logged to be onClick! The issue is with tracking onboarding.setup_messaging_integration_button_rendered, because I believe this can only be logged when the button is rendered.

malwilley · 2024-08-06T22:17:39Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+};
+
+function SetupMessagingIntegrationButton({
+ organization,


Instead of passing in organization, you can use useOrganization() inside this component. Makes it so that you don't need to pass that prop around everywhere.

In your test, you can add your test org to the context by doing:

render(<Component />, { organization: mockOrganization })

saponifi3d · 2024-08-06T22:40:44Z

static/app/views/alerts/rules/issue/index.tsx

+ {...(organization.features.includes(
+ 'messaging-integration-onboarding'
+ )


i'd recommend pulling the organization.features.includes('messaging-integration-onboarding') into a variable like: hasMessagingIntegration or isMessagingIntegrationEnabled and use that variable instead of checking for the features each time.

saponifi3d · 2024-08-06T22:41:49Z

static/app/views/alerts/rules/issue/index.tsx

+ {...(organization.features.includes(
+ 'messaging-integration-onboarding'
+ )
+ ? {


rather than making this a ternary, could we just use && logic since there isn't an else case?

saponifi3d · 2024-08-06T22:43:55Z

static/app/views/alerts/rules/issue/messagingIntegrationModal.tsx

@@ -32,6 +33,13 @@ function MessagingIntegrationModal({
 project,
 onAddIntegration,
 }: Props) {
+ useEffect(() => {
+ trackAnalytics('onboarding.messaging_integration_modal_rendered', {


+1 to the button clicks - just wanted to also thank you for that link! i've been hoping to find something like that! 🎉

sentaur-athena · 2024-08-07T16:45:58Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+
+ useEffect(() => {
+ if (project && !project.hasAlertIntegrationInstalled) {
+ trackAnalytics('onboarding.messaging_integration_button_rendered', {


@malwilley I saw your other comment about not having logs in modal render but this button we really want analytics for. The way I read this code is that because it's in useEffect and only depends on project and organization this useEffect will be only called once. What would make it be double logged?

Since project is an object, if it were to be refetched the reference would change (even if the actual values didn't) which would trigger this useEffect again. Just pointing out that useEffect can easily result in more runs than you would expect.

Since there is no action the user takes to get to this state, another way you could log this data is by using useRouteAnalyticsParams to append a new property to the page view analytic. This is a reliable way of tracking things that are shown to the user: https://develop.sentry.dev/development/analytics/#route-based-frontend-analytics

Thank you. Will look into it ❤️

saponifi3d · 2024-08-07T17:21:31Z

static/app/views/alerts/rules/issue/messagingIntegrationModal.tsx

@@ -13,11 +13,11 @@ import AddIntegrationRow from 'sentry/views/alerts/rules/issue/addIntegrationRow
 import {IntegrationContext} from 'sentry/views/settings/organizationIntegrations/integrationContext';

 type Props = ModalRenderProps & {
- headerContent: React.ReactElement<any, any>;
+ headerContent: string;


nit:

Suggested change

headerContent: string;

headerContent: React.ReactNode;

which is: string | null | JSX.Element (basically, it's a catch all for how you're using this variable and the bodyContent variable. and is closer to the React.ReactElement type from before)

saponifi3d

LGTM 🎉

saponifi3d · 2024-08-07T18:12:54Z

static/app/views/alerts/rules/issue/setupMessagingIntegrationButton.tsx

+ return null;
+ }
+
+ // TODO(Mia): only render if organization has team plan and above


just want to make sure it's okay to merge w/ this todo.

Yup! That will be in my next PR, it's fine for now since this button is hidden behind a feature flag :)

sentry-io · 2024-08-07T21:50:39Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ Error: Bad argument type. Starting with v5, only the "Object" form is allowed when calling query related... useApiQuery(queryClient.tsx) View Issue

_{Did you find this useful? React with a 👍 or 👎}

getsentry-bot · 2024-08-07T22:01:13Z

PR reverted: 064a0e0

This reverts commit 26cb3d1. Co-authored-by: ameliahsu <55610339+ameliahsu@users.noreply.github.com>

saponifi3d · 2024-08-07T22:07:36Z

^ thanks for the quick action and investigation on that failure. is unrelated. 🎉

This reverts commit 064a0e0.

reverting revert (#75679)

add logging + functional button component

57c7107

ameliahsu requested a review from a team August 6, 2024 20:27

ameliahsu requested a review from a team as a code owner August 6, 2024 20:27

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 6, 2024

ameliahsu requested a review from malwilley August 6, 2024 21:18

malwilley reviewed Aug 6, 2024

View reviewed changes

saponifi3d reviewed Aug 6, 2024

View reviewed changes

fixes

7258198

ameliahsu requested a review from saponifi3d August 7, 2024 16:39

vercel bot deployed to Preview August 7, 2024 16:41 View deployment

sentaur-athena reviewed Aug 7, 2024

View reviewed changes

saponifi3d reviewed Aug 7, 2024

View reviewed changes

more fix

e4c6996

vercel bot deployed to Preview August 7, 2024 17:34 View deployment

ameliahsu requested a review from saponifi3d August 7, 2024 18:08

saponifi3d approved these changes Aug 7, 2024

View reviewed changes

ameliahsu merged commit 26cb3d1 into master Aug 7, 2024
43 checks passed

ameliahsu deleted the mia/onboarding/analytics branch August 7, 2024 18:15

ameliahsu added the Trigger: Revert add to a merged PR to revert it (skips CI) label Aug 7, 2024

getsentry-bot added a commit that referenced this pull request Aug 7, 2024

Revert "feat(onboarding): add logging (#75679)"

064a0e0

This reverts commit 26cb3d1. Co-authored-by: ameliahsu <55610339+ameliahsu@users.noreply.github.com>

ameliahsu added a commit that referenced this pull request Aug 7, 2024

Revert "Revert "feat(onboarding): add logging (#75679)""

c4a325e

This reverts commit 064a0e0.

ameliahsu mentioned this pull request Aug 7, 2024

feat(onboarding): add logging #75782

Merged

ameliahsu added a commit that referenced this pull request Aug 7, 2024

feat(onboarding): add logging (#75782)

a3f2456

reverting revert (#75679)

github-actions bot locked and limited conversation to collaborators Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(onboarding): add logging #75679

feat(onboarding): add logging #75679

ameliahsu commented Aug 6, 2024

codecov bot commented Aug 6, 2024 •

edited

Loading

malwilley Aug 6, 2024

malwilley Aug 6, 2024

malwilley Aug 6, 2024

malwilley Aug 6, 2024

malwilley Aug 6, 2024

malwilley Aug 6, 2024 •

edited

Loading

saponifi3d Aug 6, 2024

ameliahsu Aug 7, 2024

saponifi3d Aug 7, 2024

ameliahsu Aug 7, 2024

malwilley Aug 6, 2024

saponifi3d Aug 6, 2024

saponifi3d Aug 6, 2024

saponifi3d Aug 6, 2024

sentaur-athena Aug 7, 2024

malwilley Aug 7, 2024 •

edited

Loading

sentaur-athena Aug 7, 2024

saponifi3d Aug 7, 2024 •

edited

Loading

saponifi3d left a comment

saponifi3d Aug 7, 2024

ameliahsu Aug 7, 2024

sentry-io bot commented Aug 7, 2024

getsentry-bot commented Aug 7, 2024

saponifi3d commented Aug 7, 2024

feat(onboarding): add logging #75679

feat(onboarding): add logging #75679

Conversation

ameliahsu commented Aug 6, 2024

codecov bot commented Aug 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malwilley Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malwilley Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Aug 7, 2024

Suspect Issues

getsentry-bot commented Aug 7, 2024

saponifi3d commented Aug 7, 2024

codecov bot commented Aug 6, 2024 •

edited

Loading

malwilley Aug 6, 2024 •

edited

Loading

malwilley Aug 7, 2024 •

edited

Loading

saponifi3d Aug 7, 2024 •

edited

Loading