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

[🐛] Analytics - cannot log custom parameters #4594

Closed
3 of 10 tasks
ibobo opened this issue Nov 23, 2020 · 7 comments · Fixed by #5811
Closed
3 of 10 tasks

[🐛] Analytics - cannot log custom parameters #4594

ibobo opened this issue Nov 23, 2020 · 7 comments · Fixed by #5811
Labels
help: good-first-issue Issues that are non-critical issues & well defined for first time contributors. Keep Open avoids the stale bot platform: javascript plugin: analytics Google Analytics for Firebase tools: typings TypeScript / Flow type: bug New bug report Workflow: Needs Review Pending feedback or review from a maintainer.

Comments

@ibobo
Copy link
Contributor

ibobo commented Nov 23, 2020

🔥

Issue

Other Firebase platforms let me log custom dimensions and metrics alongside standard events, like a section inside of a screen_view event, but RNFirebase does not.

When trying to call logScreenView with custom parameters, the library gives an firebase.analytics().logScreenView()*: unknown property 'X'. error.


Project Files

Doesn't require special setup other than adding Analytics package.

Javascript

Click To Expand
analytics().logScreenView({
  screen_class: currentRouteName,
  screen_name: currentRouteName,
  section: sectionName,
});

package.json:

# N/A

firebase.json for react-native-firebase v6:

# N/A

iOS

Click To Expand

ios/Podfile:

  • I'm not using Pods
  • I'm using Pods and my Podfile looks like:
# N/A

AppDelegate.m:

// N/A


Android

Click To Expand

Have you converted to AndroidX?

  • my application is an AndroidX application?
  • I am using android/gradle.settings jetifier=true for Android compatibility?
  • I am using the NPM package jetifier for react-native compatibility?

android/build.gradle:

// N/A

android/app/build.gradle:

// N/A

android/settings.gradle:

// N/A

MainApplication.java:

// N/A

AndroidManifest.xml:

<!-- N/A -->


Environment

Click To Expand

react-native info output:

System:
    OS: macOS 10.15.7
    CPU: (8) x64 Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
    Memory: 1.51 GB / 16.00 GB
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 12.19.0 - /usr/local/bin/node
    Yarn: 1.22.5 - ~/.yarn/bin/yarn
    npm: 6.14.8 - /usr/local/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.10.0 - /usr/local/lib/ruby/gems/2.6.0/bin/pod
  SDKs:
    iOS SDK:
      Platforms: iOS 14.2, DriverKit 20.0, macOS 11.0, tvOS 14.2, watchOS 7.1
    Android SDK:
      API Levels: 21, 23, 27, 28, 29
      Build Tools: 23.0.3, 28.0.3, 29.0.2, 29.0.3
      System Images: android-19 | Google APIs Intel x86 Atom, android-23 | Google APIs Intel x86 Atom, android-25 | Google APIs ARM 64 v8a, android-26 | Google APIs Intel x86 Atom_64, android-27 | Google Play Intel x86 Atom, android-28 | Intel x86 Atom_64, android-28 | Google Play Intel x86 Atom, android-29 | Google Play Intel x86 Atom_64
      Android NDK: Not Found
  IDEs:
    Android Studio: 4.0 AI-193.6911.18.40.6626763
    Xcode: 12.2/12B45b - /usr/bin/xcodebuild
  Languages:
    Java: 1.8.0_77 - /usr/bin/javac
    Python: 2.7.16 - /usr/bin/python
  npmPackages:
    @react-native-community/cli: Not Found
    react: ^16.13.1 => 16.13.1 
    react-native: 0.63.3 => 0.63.3 
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found
  • Platform that you're experiencing the issue on:
    • iOS
    • Android
    • iOS but have not tested behavior on Android
    • Android but have not tested behavior on iOS
    • Both
  • react-native-firebase version you're using that has this issue:
    • 10.0.0
  • Firebase module(s) you're using that has the issue:
    • @react-native-firebase/analytics: 10.0.0
  • Are you using TypeScript?
    • Y & 4.1.2


@ibobo ibobo added help: needs-triage Issue needs additional investigation/triaging. type: bug New bug report labels Nov 23, 2020
@mikehardy
Copy link
Collaborator

Hi there!
Can you point to the documentation where this is allowed? I thought our wrap of the underlying APIs matched the firebase-js-sdk declaration exactly and we were just validating their formal types

I think logScreenView is deprecated and moved, as a side note and you want setCurrentScreen

https://firebase.googleblog.com/2020/08/google-analytics-manual-screen-view.html

https://github.com/firebase/firebase-js-sdk/blob/3d5354021abaedcc108e0c960026990f5c5eda26/packages/analytics-types/index.d.ts#L56

@ibobo
Copy link
Contributor Author

ibobo commented Nov 23, 2020

Hi Mike! The docs doesn't explicitly allows passing extra params, but it's possible.
In fact:

  • if you scroll through the first link you provided, you'll see they're adding a MyAppAnalyticsParameterFitnessCategory: category! parameter on their calls to logEvent for screenView;
  • the TypeScript typings for the firebase-js-sdk, and RNFirebase's own also, check for the availability of the two required params, but don't prohibit "extraneous" ones.

Also it's worth mentioning that I'm doing this in another web only Firebase project, and the Analytics reports give me stats about the other params in the single event panel as expected.

On the logScreenView/setCurrentScreen issue: it's actually the opposite. This is the issue where it was deprecated, but now it's actually removed:
#4145

@mikehardy
Copy link
Collaborator

Sorry for the delay - I had a chance to think about this a bit more just now

  • interesting, yes they expect those two things in general but then allow semi-arbitrary other things in the event hit
  • totally right about logScreenView vs setCurrentScreen - logScreenView is the new new, setCurrentScreen is gone

Okay, so this particular area of the code is pretty heavily engineered, so making the change will in the end be a really small change I think, but involve a lot of domain knowledge

Analytics uses the "superstruct" dependency to define (and then validate) the object shapes, the 'struct' method that is called during these definitions is exported here:

export default superstruct({

superstruct apparently supports a way of defining some fixed things (and validating them) while allowing arbitrary other things: https://docs.superstructjs.org/resources/faq#how-can-i-allow-unknown-keys-with-object-structs

If you can bend the definitions of structs.ScreenView such that it passes validation correctly (screen class and name are still checked etc) and takes the extra parameters I will happily merge it

Alternatively (and maybe actually easier though it is more typing - literally typing keys, but also typescript typing) expand the parameters to include the upstream list: https://github.com/firebase/firebase-js-sdk/blob/3d5354021abaedcc108e0c960026990f5c5eda26/packages/analytics-types/index.d.ts#L125-L153

That second style might be better as it is strongly typed, although the first style is potentially less fragile as we wouldn't have to stay in sync with firebase-js-sdk - tradeoffs in both directions. I think I might prefer strong typing really, as that's the point of typescript, even if it makes things a bit more fragile as we'll be dependent on their definitions.

@mikehardy mikehardy added plugin: analytics Google Analytics for Firebase tools: typings TypeScript / Flow Workflow: Waiting for User Response Blocked waiting for user response. help: good-first-issue Issues that are non-critical issues & well defined for first time contributors. platform: javascript and removed help: needs-triage Issue needs additional investigation/triaging. labels Nov 28, 2020
@stale
Copy link

stale bot commented Jan 3, 2021

Hello 👋, to help manage issues we automatically close stale issues.
This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the Type: Stale Issue has become stale - automatically added by Stale bot label Jan 3, 2021
@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. Keep Open avoids the stale bot and removed Type: Stale Issue has become stale - automatically added by Stale bot Workflow: Waiting for User Response Blocked waiting for user response. labels Jan 19, 2021
@todesignandconquer
Copy link

Hi, is there any update on this? Would love to be able to start tracking custom metrics and AFAICT there is not a clear way to achieve this.

@mikehardy
Copy link
Collaborator

All the dev in this repo is out in the open and you're the first comment in a while, so you are the update. Do you have a PR in mind? Happy to collaborate to help get it merged

tomonacci added a commit to tomonacci/react-native-firebase that referenced this issue Oct 25, 2021
tomonacci added a commit to tomonacci/react-native-firebase that referenced this issue Oct 25, 2021
mikehardy added a commit that referenced this issue Oct 25, 2021
…5811)

* fix(analytics): allow custom event parameters for screen_view events

Fixes #4594.

* style(lint): fix prettier error

Co-authored-by: Mike Hardy <github@mikehardy.net>
@mikehardy
Copy link
Collaborator

@tomonacci generously looked into this and provided a PR to fix it! Will be out in the next release (usually near the end of the week, every week) 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help: good-first-issue Issues that are non-critical issues & well defined for first time contributors. Keep Open avoids the stale bot platform: javascript plugin: analytics Google Analytics for Firebase tools: typings TypeScript / Flow type: bug New bug report Workflow: Needs Review Pending feedback or review from a maintainer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants