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(analytics): add method to retrieve the app instance id from the … #5210

Merged

Conversation

gabrielbull
Copy link
Contributor

@gabrielbull gabrielbull commented Apr 24, 2021

…service

Description

Added method to retrieve the app instance id from the service. This is necessary when triggering GA events from a backend using the Measurement Protocol.

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Tested on both Android and iOS.


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Apr 24, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/3aL7xezMzhjb4SVZspM1weTyWTrb
✅ Preview: https://react-native-f-git-fork-gabrielbull-feat-analytics-app-i-199d03.vercel.app

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #5210 (f8df328) into master (589fcb0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head f8df328 differs from pull request most recent head 3ec776a. Consider uploading reports for the commit 3ec776a to get more accurate results

@@            Coverage Diff             @@
##           master    #5210      +/-   ##
==========================================
+ Coverage   88.87%   88.87%   +0.01%     
==========================================
  Files         109      109              
  Lines        3744     3745       +1     
  Branches      360      360              
==========================================
+ Hits         3327     3328       +1     
  Misses        370      370              
  Partials       47       47              

@mikehardy
Copy link
Collaborator

Hi there! Thanks for this @gabrielbull - first let me say this is probably the cleanest first contribution I have ever seen, seriously, and that's a big help, thank you.

I'm a bit new to the GA4 thing - it appears that this is the relevant explainer for how the instance id is used, am I correct in thinking so?

https://support.google.com/analytics/answer/9213390?hl=en

If so it appears that setting the user-id would bet the first way to identify a user across properties, and the app instance id will be used as a fall back in certain analytics configurations? And you want to retrieve it here in order to pass it to e.g. cloud functions (or really any non-app location where you also want to log events related to this user?)

I think I'm understanding things but I want to make certain, and then I'll just re-read the docs and maybe add that link and a bit more explaining for anyone else coming at it with semi-fresh or totally fresh eyes, as I was

Otherwise looks almost good to go - I just approved the CI workflows so they are chewing through it now

@mikehardy mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Apr 24, 2021
@gabrielbull
Copy link
Contributor Author

gabrielbull commented Apr 24, 2021

Hi @mikehardy. It's actually a bit different than the link you sent. It is a required parameter for sending events to the Google Analytics Measurement Protocol.

Here is a cool tool you can use to create events and validate them using the Measurement Protocol Validation Server.

For my specific use case (there are infinite use cases I'm sure), I'm using a Stripe webhooks to send purchase/subscription events to GA and this allows me to see the revenue on GA. This can't be done on the frontend only as the subscriptions are renewed by Stripe automatically.

I just noticed my PR failed ESLint due to an extra space, so I just removed it and force-pushed my fixed commit. I believe you have to trigger the workflows again unfortunately.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I thought about maybe putting more information (like a link to the doc you sent) explaining this would be needed for GA4 but I imagine since it's strictly required there and offered here with the obvious name that anyone who needs it won't have a feature discovery issue. So I think this is good to go right out of the gate and I learned some GA4 stuff. Great

@mikehardy
Copy link
Collaborator

Our CI has slowly fallen prey to entropy so it takes me a few attempts to get a clean run sometimes but this is marked for merge as soon as CI goes green and will go out as either v11.3.4 o whatever version is next. I literally just did a release tonight and I'd like to gather a bit more change before another, but knowing this will merge you should feel free to use the auto-generated patch-package set to integrate this locally until the new version comes out and you may evict them, if you don't already have a local patch installed - https://github.com/invertase/react-native-firebase/pull/5210/checks?check_run_id=2425065299

Thanks again

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Apr 24, 2021
@gabrielbull
Copy link
Contributor Author

gabrielbull commented Apr 24, 2021

Awesome news 🎉!

@mikehardy mikehardy merged commit a51e97b into invertase:master Apr 27, 2021
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Apr 27, 2021
@mikehardy
Copy link
Collaborator

Release 11.4.1 just went out with the change related to this among other fixes + features. Enjoy! 🚀

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