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(INT-261): refactor ga4 device mode #1185

Merged
merged 24 commits into from
Aug 23, 2023
Merged

Conversation

mihir-4116
Copy link
Contributor

@mihir-4116 mihir-4116 commented Jun 26, 2023

PR Description

  • This pull request implements a comprehensive refactor of the GA4 integration. The refactoring follows the same approach as the GA4 Cloud Mode flow. All the changes made in this pull request are backward compatible.

  • Key highlights of this pull request include:

  1. Introducing new functions:
  • getValueFromMessage: Retrieves a value from the message payload.
  • handleMetadataForValue: Handles metadata for a specific value.
  • formatValues: Formats the values to prepare integration payload in a common utility file.
  • handleSourceKeysOperation: Handles the operation on source keys.
  1. Enhancements to existing functions:
  • constructPayload: Modified to prepare the GA4 integration payload, incorporating the newly introduced functions.
  • The code for the newly introduced functions is identical to the transformer function.
  1. Simplified page call based on toggle:
  • If the extendPageViewParams toggle is enabled, the page call will include properties.
  • If the toggle is disabled, the page call will include only the standard payload.
  1. Removal of capturePageView option from the identify call:
  • The capturePageView option is no longer required in the identify call.

  • Please refer to the Cloud Mode GA4 integration track call flow for a better understanding of the changes in device mode
    track calls.

  • Additionally, this pull request includes:

    • Unit tests for all utility functions.
    • Unit tests for all GA4 and RudderStack standard e-commerce events.
  • These changes improve the overall structure, performance, and maintainability of the GA4 integration codebase while
    ensuring compatibility with existing functionality.

  1. Removed dependency from analytics apis at event level

Notion ticket

Ticket link

Screenshots

Please add screenshots for any new features or UI bug fixes for the following browsers -

  • Chrome
  • Firefox
  • Safari

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@mihir-4116 mihir-4116 self-assigned this Jun 26, 2023
@mihir-4116 mihir-4116 marked this pull request as draft June 26, 2023 11:09
@github-actions
Copy link

github-actions bot commented Jun 26, 2023

size-limit report 📦

Name Size (Base) Size (Current) Size Limit Status
Core - CDN 36.56 KB 36.56 KB (-0.01% ▼) 40.04 KB
All Integrations - CDN 426.61 KB 431.71 KB (+1.2% ▲) 433.01 KB
Core - NPM 36.51 KB 36.51 KB (-0.02% ▼) 39.94 KB
Service Worker - NPM 27.8 KB 27.8 KB (0%) 27.83 KB

@mihir-4116 mihir-4116 marked this pull request as ready for review June 28, 2023 07:34
src/utils/utils.js Outdated Show resolved Hide resolved
src/utils/utils.js Outdated Show resolved Hide resolved
.size-limit.js Outdated Show resolved Hide resolved
src/utils/utils.js Outdated Show resolved Hide resolved
saikumarrs
saikumarrs previously approved these changes Aug 18, 2023
@mihir-4116 mihir-4116 changed the title chore: refactor ga4 device mode chore(INT-123): refactor ga4 device mode Aug 18, 2023
@linear
Copy link

linear bot commented Aug 18, 2023

INT-123 GA4 docs inconsistency between Device mode and cloud mode

Hi Team,
Our GA4 docs have an inconsistency between Device mode and cloud mode

  1. ga4 docs are not consistent and don't call out currency as a required field for device mode whereas on cloud mode it is listed as a required field.
  2. The ecomm spec doesn't have currency in product added in the Product added section where as it is a mandatory field on GA4. https://www.rudderstack.com/docs/event-spec/ecommerce-events-spec/ordering/#product-added
    Ex from the doc:
  product_id: "123",
  sku: "F15",
  category: "Games",
  name: "Game",
  brand: "Gamepro",
  variant: "111",
  price: 13.49,
  quantity: 11,
  coupon: "DISC21",
  position: 1,
  url: "https://www.website.com/product/path",
  image_url: "https://www.website.com/product/path.webp",
})```
Suggestion:
We can we make USD a default value for currency for the device mode, it's already the default for the cloud mode.
david

Slack Link - https://rudderlabs.slack.com/archives/C02A3UAU9UN/p1690393744137139
Channel - ask-integrations
Created By - David Daly

-------
# 1. Problem

# 2. Impact

Who are affected customers? Are there more than the ones who reported?
How are we unblocking and notifying them?
How it could've been avoided?

# 3. Alerts

*Please list here the alert received and the definition of the alert for discussion. Also please add any missing metrics and alerts that would have helped in this case to the action items*

# 4. Time Taken

💡Issue happened ==> (Time to detect) ==> Issue Notified ==> (Time to root cause) ==> Issue Diagnosed ==> (Time to resolve) ==> Patch applied

Time to detect - ?
Time to root cause - ?
Time to resolve - ?

# 5. Lessons Learned
**What went well:**
- *eg. Monitoring quickly alerted us to high rate (reaching ~100%) of HTTP 500s*
**What went wrong:**
- *eg. No runbook available to handle this kind of cascading failure*
**Where we got lucky:**
- eg. Server logs had stack traces pointing to file descriptor exhaustion as cause for crash

# 6. Debugging

# 7. Action Items and ETA

# 8.  Appendix

@mihir-4116 mihir-4116 changed the title chore(INT-123): refactor ga4 device mode chore(INT-261): refactor ga4 device mode Aug 18, 2023
@linear
Copy link

linear bot commented Aug 18, 2023

@koladilip
Copy link
Contributor

Details are explained well @mihir-4116, Good job

saikumarrs
saikumarrs previously approved these changes Aug 18, 2023
@utsabc
Copy link
Member

utsabc commented Aug 21, 2023

utsabc
utsabc previously approved these changes Aug 21, 2023
src/integrations/GA4/utils.js Outdated Show resolved Hide resolved
src/integrations/GA4/utils.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Aug 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

73.0% 73.0% Coverage
0.0% 0.0% Duplication

@mihir-4116
Copy link
Contributor Author

https://sonarcloud.io/component_measures?metric=new_coverage&selected=rudderlabs_rudder-sdk-js%3Asrc%2Fintegrations%2FGA4%2Fbrowser.js&view=list&pullRequest=1185&id=rudderlabs_rudder-sdk-js

Can we add more coverage to browser.js ?

Increased browser file coverage from 43% to 83%

@mihir-4116 mihir-4116 added the Ready for review Changes are ready to be reviewed label Aug 23, 2023
@mihir-4116 mihir-4116 merged commit 48bc582 into production-staging Aug 23, 2023
9 checks passed
@mihir-4116 mihir-4116 deleted the feat/ga4-v2 branch August 23, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review Changes are ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants