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

Merged
merged 3 commits into from
Oct 22, 2024
Merged

chore: add Heap analytics #6951

merged 3 commits into from
Oct 22, 2024

Conversation

peterbarnett03
Copy link
Contributor

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.

@wdoconnell wdoconnell requested a review from blegesse-w October 2, 2024 14:16
Copy link
Contributor 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.

blegesse-w
blegesse-w previously approved these changes Oct 14, 2024
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.

this looks good to me. Anything else needs to be in place before merging?

@wdoconnell
Copy link
Contributor

this looks good to me. Anything else needs to be in place before merging?

Pending a final approval in https://github.com/influxdata/granite/issues/3358.

@wdoconnell wdoconnell added this pull request to the merge queue Oct 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2024
@wdoconnell
Copy link
Contributor

CircleCI status page indicates jobs are failing. I'll wait until those resolve before re-merging.

@wdoconnell wdoconnell added this pull request to the merge queue Oct 22, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 22, 2024
* chore: add Heap analytics

* feat: unload analytics script on fail, use id appropriate to environment

* fix: correct capitalization of heapanalyticsid

---------

Co-authored-by: Peter Barnett <[email protected]>
Co-authored-by: Bill OConnell <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2024
@wdoconnell wdoconnell added this pull request to the merge queue Oct 22, 2024
Merged via the queue into master with commit a2cea56 Oct 22, 2024
6 checks passed
@wdoconnell wdoconnell deleted the addHeap branch October 22, 2024 19:33
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