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

chore: add Heap analytics #6951

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore: add Heap analytics #6951

wants to merge 2 commits into from

Conversation

peterbarnett03
Copy link

This is re-enabling some UX analytics software so we can better understand usage patterns for future UI/UX updates across the InfluxData suite of products.

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@peterbarnett03 peterbarnett03 marked this pull request as ready for review September 18, 2024 00:51
@peterbarnett03 peterbarnett03 requested review from a team as code owners September 18, 2024 00:51
Copy link
Member

@blegesse-w blegesse-w left a comment

Choose a reason for hiding this comment

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

Hi @peterbarnett03, I'll prioritize this and take a look at it today. In the meantime, do you know why the monitor-ci job is failing? Haven't had a chance to investigate

@wdoconnell
Copy link
Contributor

I'll pick this one up as part of the internal ticket we have for this. I think the CI failure is related to the mon ci runner changes we made recently so I can take a look. 👍

@wdoconnell
Copy link
Contributor

@blegesse-w @peterbarnett03 This should be ready for a re-review. I added a few things here to what Pete already included:

  1. Moved the heap identifiers to a ConfigCat int flag so that they stay out of code, and so we can control the staging/production identifiers through ConfigCat.
  2. Cleans up the window object and script tags if the script fails.
  3. Added some explanatory comments where necessary.

Overall I didn't see any regressions in behavior when I tested it local. This is less temperamental than the VWO script and the fetched scripts don't appear to interfere with page styles.

Copy link
Author

@peterbarnett03 peterbarnett03 left a comment

Choose a reason for hiding this comment

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

LGTM -- appreciate you moving the ID to a dynamic flag.

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.

3 participants