-
Notifications
You must be signed in to change notification settings - Fork 808
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
[WooCommerce Analytics] Add clickhouse param to record events in w.gif #41476
base: release/woocommerce-analytics-0.4.2
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on adding w.gif for ClickHouse.
verify t.gif and w.gif are loaded without
woocommerceanalytics_
prefix on the event name in w.gif but yes in t.gif
I think you meant the parameter _en
should include the prefix or not, right?
I saw both w.gif
and t.gif
's _en
param still contain the prefix, do we need to add that logic in this package in favour of this discussion https://github.com/Automattic/analytics/pull/76#discussion_r1937616615?
Perform some events For example My Account View. And verify only t.gif is loaded
When I went to "My Account" page I didn't see t.gif
was loaded, only g.gif
. It's also true if I didn't apply the change from this PR.
Let's see how the discussion ends. |
…4.2' into add/clickhouse-param # Conflicts: # projects/packages/woocommerce-analytics/src/class-woo-analytics-trait.php
Cart page ✅
My Account pageOnly I think maybe it's not related to this PR because when I used the real |
Don't know about that g.gif but are you logged in as a non admin? |
Yeah, I opened an incognito window and did not log in any user. |
It seems that is needed to have a logged in user to trigger my account page view event, just entering the URL that shows the register or login form won't trigger the event. |
You're right, |
@ianlin After discussions here https://github.com/Automattic/analytics/pull/76 and with systems we don't need to remove So in brief all the events with 'ch=1' param in the query will be sent as well to CH. THis is ready for a final round. Thanks in advance |
Fixes #
Proposed changes:
This PR needs to be tested in parallel with https://github.com/Automattic/analytics/pull/76
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: