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

upcoming: [M3-8328] - Add Analytics Events to Linode Create v2 #10649

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jul 8, 2024

Description 📝

  • This PR adds custom analytics events to Linode Create v2
  • This PR's goal is to copy over only events UX wanted to retain from Linode Create v1
    • View this doc to see what events are kept / not kept 📄
    • The events that are retained should work the same way as they did on Linode Create v1

How to test 🧪

Prerequisites

  • Enable Linode Create v2 feature flag 🇺🇸
  • Run _satellite.setDebug(true) in your browser's console ⌨️
  • Refresh the page 🔄

Verification steps

  • Create various Linodes on different tabs of the Linode Create flow
    • Verify Analytics events fire for each creation
    • Verify those analytics events match what Linode Create v1 would capture

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@bnussman-akamai bnussman-akamai added Analytics Relating to Analytics migration project or Adobe Analytics Linode Create Refactor labels Jul 8, 2024
@bnussman-akamai bnussman-akamai self-assigned this Jul 8, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review July 8, 2024 15:39
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner July 8, 2024 15:39
@bnussman-akamai bnussman-akamai requested review from jaalah-akamai, abailly-akamai and mjac0bs and removed request for a team July 8, 2024 15:39
Copy link

github-actions bot commented Jul 8, 2024

Coverage Report:
Base Coverage: 82.23%
Current Coverage: 82.23%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks! I confirmed sendLinodePowerOffEvent, sendApiAwarenessClickEvent, sendLinodeCreateFlowDocsClickEvent, and sendLinodeCreateEvent were still firing and the other events are no longer being tracked.

Aside from sendLinodeCreate on Clone, everything fired as expected.

@bnussman-akamai bnussman-akamai requested a review from mjac0bs July 8, 2024 18:20
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

🚢

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Jul 8, 2024
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Confirming seeing proper events being fired ✅

if (type === 'Clone Linode' && values.linode) {
// @todo use Linode Query key factory when implemented
const linode = await queryClient.ensureQueryData({
queryFn: () => getLinode(values.linode!.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid this assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish. Not sure why Typescript can't see the values.linode check in the if statement

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the type checker does not refine the type of values.linode inside the lambda function passed to queryFn.
just make it a new const

if (type === 'Clone Linode' && values.linode) {
    // @todo use Linode Query key factory when implemented
    const linodeId = values.linode.id;
    const linode = await queryClient.ensureQueryData({
      queryFn: () => getLinode(linodeId),
      queryKey: ['linodes', 'linode', linodeId, 'details'],
    });

    sendCreateLinodeEvent('clone', values.type, {
      isLinodePoweredOff: linode.status === 'offline',
    });
  }

@@ -113,7 +113,7 @@ export const sendCreateNodeBalancerEvent = (eventLabel: string): void => {
// LinodeCreateContainer.tsx
export const sendCreateLinodeEvent = (
eventAction: string,
eventLabel: string,
eventLabel: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still send an event if we don't have a label?

Copy link
Member Author

@bnussman-akamai bnussman-akamai Jul 8, 2024

Choose a reason for hiding this comment

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

v1 does, so I opted to send even for undefined. I had to update the type because v1 just lied about types

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jul 8, 2024
@bnussman-akamai bnussman-akamai merged commit 22a8bcf into linode:develop Jul 8, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analytics Relating to Analytics migration project or Adobe Analytics Approved Multiple approvals and ready to merge! Linode Create Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants