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

Add (unstable) telemetry support with a hook #2368

Merged
merged 7 commits into from
Oct 31, 2019
Merged

Add (unstable) telemetry support with a hook #2368

merged 7 commits into from
Oct 31, 2019

Conversation

amrocha
Copy link
Contributor

@amrocha amrocha commented Oct 25, 2019 •
edited
Loading

WHY are these changes introduced?

We want to gather data around icon usage, and test the concept of gathering data about component usage, before deciding if we want to expand this to other components.

This is a temporary change, so we will remove it in a few weeks, once we have gathered enough data. However, we want feedback on this approach for gathering usage data on Polaris components.

WHAT is this pull request doing?

This PR adds a prop to the AppProvider that is then accessed by Icon (for now, but possibly any component in the future if this approach works) via a hook. Icon then logs relevant information to monorail when it is rendered.

In order to gather data, this PR will be accompanied by a PR in web that provides their monorail instance to AppProvider, and that we can then use in polaris-react to log usage.
Adding

In order to clearly mark this as a temporary change, we marked all public-facing aspects of it "unstable" cc @tmlayton

How to 🎩

See the example code

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';
import {
  PlusMinor,
  AlertMinor,
  ArrowDownMinor,
  ArrowLeftMinor,
  ArrowRightMinor,
} from '@shopify/polaris-icons';
import {
  Page,
  AppProvider,
  Card,
  ResourceList,
  Avatar,
  TextStyle,
  Icon,
} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <AppProvider
        i18n={{
          Polaris: {
            ResourceList: {
              sortingLabel: 'Sort by',
              defaultItemSingular: 'item',
              defaultItemPlural: 'items',
              showing: 'Showing {itemsCount} {resource}',
              Item: {
                viewItem: 'View details for {itemName}',
              },
            },
            Common: {
              checkbox: 'checkbox',
            },
          },
        }}
        UNSTABLE_telemetry={{
          produce: (schemaId, payload) => {
            console.log(schemaId, payload);
            return true;
          },
        }}
      >
        <Page>
          <Card>
            <Icon source={PlusMinor} />
            <Icon source={AlertMinor} />
            <Icon source={ArrowDownMinor} />
            <Icon source={ArrowLeftMinor} />
            <Icon source={ArrowRightMinor} />
            <ResourceList
              showHeader
              items={[
                {
                  id: 341,
                  url: 'customers/341',
                  name: 'Mae Jemison',
                  location: 'Decatur, USA',
                },
                {
                  id: 256,
                  url: 'customers/256',
                  name: 'Ellen Ochoa',
                  location: 'Los Angeles, USA',
                },
              ]}
              renderItem={(item) => {
                const {id, url, name, location} = item;
                const media = <Avatar customer size="medium" name={name} />;

                return (
                  <ResourceList.Item id={id} url={url} media={media}>
                    <h3>
                      <TextStyle variation="strong">{name}</TextStyle>
                    </h3>
                    <div>{location}</div>
                  </ResourceList.Item>
                );
              }}
            />
          </Card>
        </Page>
      </AppProvider>
    </Page>
  );
}

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2019 •
edited
Loading

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified6
Files potentially affected89

Details

All files potentially affected (total: 89)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AppProvider/AppProvider.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Icon/Icon.tsx (total: 85)

Files potentially affected (total: 85)

🧩 src/utilities/telemetry/context.tsx (total: 89)

Files potentially affected (total: 89)

🧩 src/utilities/telemetry/hooks.tsx (total: 88)

Files potentially affected (total: 88)

🧩 src/utilities/telemetry/index.ts (total: 87)

Files potentially affected (total: 87)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

amrocha
amrocha commented Oct 28, 2019
View reviewed changes
BPScott
BPScott reviewed Oct 28, 2019
View reviewed changes
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Can we avoid 'monorail' in the naming - something more generic like "telemetry" feels a bit more agonostic so we can decouple us from using specific internal tooling

@kaelig
Copy link
Contributor

kaelig commented Oct 28, 2019 •
edited
Loading

In the playground code, should <AppProvider monorail={}> be <AppProvider UNSTABLE_monorail={}>?

@BPScott
Copy link
Member

BPScott commented Oct 28, 2019

@kaelig Check the props of AppProvider: UNSTABLE_monorail={}

@tmlayton
Copy link
Contributor

UNSTABLE prefix looks good, glad to see this pattern helping to get ideas out there quicker.

@tmlayton
Copy link
Contributor

Same feels from me for using a more generic term like telemetry

@amrocha amrocha changed the title Add (unstable) monorail support with a hook Add (unstable) telemetry support with a hook Oct 30, 2019
BPScott
BPScott reviewed Oct 31, 2019
View reviewed changes
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Thanks for updating the naming. A few little nitpicks, and one bigger thing about how to handle breaking the rule of hooks -that may require a deeper chat with kaelig to make sure he understands what the presence of those fields mean for our data logging

@amrocha amrocha merged commit 7bcbb02 into master Oct 31, 2019
@amrocha amrocha deleted the add-icon-tracking branch October 31, 2019 23:51
@amrocha amrocha mentioned this pull request Dec 17, 2019
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaelig kaelig kaelig left review comments

@tmlayton tmlayton tmlayton left review comments

@BPScott BPScott BPScott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amrocha @kaelig @BPScott @tmlayton