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

feat(demodata): Add GA events for demodata #18047

Merged
merged 10 commits into from
May 12, 2020
Merged

feat(demodata): Add GA events for demodata #18047

merged 10 commits into from
May 12, 2020

Conversation

ebb-tide
Copy link
Contributor

@ebb-tide ebb-tide commented May 12, 2020

Closes 7080

@ebb-tide ebb-tide changed the title Add GA events for demodata feat(demodata): Add GA events for demodata May 12, 2020
@ebb-tide ebb-tide requested review from a team, zoesteinkamp and 121watts and removed request for a team and zoesteinkamp May 12, 2020 05:06
ui/src/index.tsx Outdated
@@ -157,6 +157,7 @@ declare global {

// Older method used for pre-IE 11 compatibility
window.basepath = basepath
window.dataLayer = window.dataLayer || []
Copy link
Contributor

Choose a reason for hiding this comment

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

idk how this is supposed to work, dataLayer isn't an array in quartz

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

just bring it up because we've had a lot of false starts with this one: https://github.com/influxdata/idpe/issues/6298 and it'd be cool to add new context on this one

Copy link
Contributor

Choose a reason for hiding this comment

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

that's def newer than the array. for sure. im fine with either. as long as they are the same.

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 window.dataLayer = [] is a fix to prevent errors from crashing the page in the event of someone calling GA.push(event) before the actual GA client has been loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand quartz and the UI interact here at all and I don't understand how GA works. if anyone does I would love some help.

Copy link
Contributor

@hoorayimhelping hoorayimhelping May 12, 2020

Choose a reason for hiding this comment

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

So we're all clear: window.dataLayer is a data structure used by Google Tag Manager. Appending an item to this array is how we "fire" events for GTM. We can view those events in google analytics or google tag manager. We're using this system absent another one because it's already in place and it's already paid for (i.e. the costs of using it are very low relative to adopting a new one, even though it's not strictly the best tool for the job).

Quartz and Cloud both use the same google tag manager system, and quartz has its own set of events that are fired. We are able to correlate user behavior between cloud and quartz, but it's not our (ui engineers) concern on how to do that right now. Basically you can view this as: as long as you don't overwrite events, everything should interact fine without much conflict

In cloud, when we deploy, we overwrite our OSS index.html file with an idpe one which sets up GTM correctly. I love the defensiveness of the line, and having it in will not hurt anything, but it's not necessary, since this same action is done in the index.html file.

https://developers.google.com/tag-manager/devguide

Copy link
Contributor

@drdelambre drdelambre May 12, 2020

Choose a reason for hiding this comment

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

for future reference and in the spirit of sharing knowledge, we haven't used that idpe file since november-ish? please refer all changes to GTM in https://github.com/influxdata/monitor-ci/blob/master/deploy/Jenkinsfile.deploy#L386

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

one small change to remove the mention of GA

ui/src/shared/utils/analytics.ts Show resolved Hide resolved
ui/src/shared/utils/analytics.ts Outdated Show resolved Hide resolved
@ebb-tide ebb-tide force-pushed the deniz-GA-dd-events branch from 3be19c8 to 42e8e13 Compare May 12, 2020 17:20
@ebb-tide ebb-tide merged commit 396e7f6 into master May 12, 2020
@jacobmarble jacobmarble deleted the deniz-GA-dd-events branch January 2, 2024 22:40
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.

4 participants