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

Bug: new crashlytics transport is not properly reporting errors #110

Open
DanielSRS opened this issue Oct 23, 2024 · 3 comments
Open

Bug: new crashlytics transport is not properly reporting errors #110

DanielSRS opened this issue Oct 23, 2024 · 3 comments

Comments

@DanielSRS
Copy link
Contributor

When I use it, I get this warning:
firebase.crashlytics().recordError(*) expects an instance of Error. Non Errors will be ignored.

Checking the implementation I noticed that props.msg is being passed down to recordError, but props.msg besides being typed as any is aways a string

try {
  if (isError) {
    props.options.CRASHLYTICS.recordError(props.msg);
  } else {
    props.options.CRASHLYTICS.log(props.msg);
  }
  return true;
} catch (error) {
  throw Error(
    `react-native-logs: crashlyticsTransport - Error on send msg to crashlytics`
  );
}

As a quick workaround I wrapped errors in an Error:

var customTransport: transportFunctionType = props => {
  const isError = props.level.text === 'error';
  const err = new Error(props.msg);

  crashlyticsTransport({ ...props, msg: isError ? err : props.msg });
};

but this is a bad alternative and customTransport ends up being reported as the source of the error.


Maybe a better approach would be to:

  • use rawMsg
  • check for any Error instances and reporting those and pass msg to CRASHLYTICS.log
  • if none, wrapping the message in an error

but thats is just an suggestion and maybe not the best solution

@alessandro-bottamedi
Copy link
Collaborator

The transport is intentionally very simple, accepting only strings, because Crashlytics is used in production, where the stack trace would be incomprehensible anyway due to the JS bundle. JavaScript source maps would need to be sent to Crashlytics, but I’m not sure if that’s currently possible…

@DanielSRS
Copy link
Contributor Author

Sorry, I think I didn't organized the info in this issue the best way. The important part is kinda hidden.

The problem is not the stack trace, but string only logs not being sent at all to crashlytcs.

When using this transport, I immediately got warnings from the firebase lib informing that those reports were going to be ignored because they're not Error instances.


I realize now that I should've investigated a little more. I started the integration with crashlytcs in my app and reported this as soon as I noticed, but collecting more data about the actual behavior would be better.

In short, what I noticed:

  • This seems to be mostly a limitation of crashlytcs being incapable of reporting strings as erros
  • this transport uses 2 crashlytcs methods: log and recordError.
    • recordError seems to only accept an Error instance as an argument, not strings.
  • for the short period of time that I investigated this (a few hours) the logs didn't seem to have reached firebase.
  • as a quick workaround I wrapped in a Error those logs that would've been passed to recordError method.
    • After I did that, the warning were gone and I could see some logs on firebase console.

Since these logs/reports can take some time to be processed by crashlytcs. I think the period of my observations were too short, and so, they can be wrong.

@ghost1face
Copy link

ghost1face commented Oct 28, 2024

I just ran into this as well, what I did was this, the comment points out the issue that was noticed:

logger.createLogger({
   transport: [ crashlyticsTransport ],
   transportOptions: {
     CRASHLYTICS: {
        recordError(msg) {
           // firebase/crashlytics expects an instance of error here
          // however, react-native-logs expects msg to be type string
           crashlyticsModule.recordError(new Error(msg));
        }
     }
   }
})

Any thoughts on supporting the latest signature of recordError? https://github.com/invertase/react-native-firebase/blob/main/packages/crashlytics/lib/index.js#L118-L128 Though I'd imagine it would be a breaking change you may not be interested in bringing in

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

No branches or pull requests

3 participants