Ignore all failures while uploading analytics data #1858
Conversation
bartekpacia
changed the title
bartekpacia
force-pushed
the
fix/simulator_runtimes_npe
branch
from
4ecdcf1
to
5b7e63b
Compare
| // This is less fine. Don't crash, but ask the user to report this. | ||
| println("Exception ocurred while uploading analytics. This is not a fatal issue, but we ask you to report it.") | ||
| println(e.message) | ||
| println(e.stackTraceToString()) |
There was a problem hiding this comment.
Why do we want to print it on cli?
I don't think this would have any meaning for user. For us yes then maybe log this in maestro.log or some log file instead.
There was a problem hiding this comment.
We want to make sure analytics data is flowing to us. If we will just swallow the exception, we will never learn if we silently break sth in analytics.
So far the only known issue w.r.t analytics is #1850 (which will be fixed by this PR), so I don't expect this to cause a lot of noise.
There was a problem hiding this comment.
I don't really see value of showing this to user. Maybe there is a better option to provide this observability then?
Users don't really care about analytics, we do. So, maybe we opt another way for this observability WDYT?
There was a problem hiding this comment.
I think the best solution would be to connect Sentry (or any other log reporting service)? That would be a lot of work though.
Actually, we already have an ErrorReporter. I just took a look at BigQuery, and it looks pretty solid. (query link - internal only)
The problem is that we don't wrap our whole app with the ErrorReporter, so not all errors are reported. This is a separate issue though.
For now I agree with you, let's remove this message for the good of our users.
There was a problem hiding this comment.
Yes you can create a separate issue for this. Its definitely useful though!
| return | ||
| } catch (e: Exception) { | ||
| // This is also fine. We don't want to bug the user. | ||
| // See discussion at https://github.com/mobile-dev-inc/maestro/pull/1858 |
There was a problem hiding this comment.
Missing return?
There was a problem hiding this comment.
it's end of the function anyway, but yeah, I added the return.
| // We don't care that much about analytics to bug user about it. | ||
| return | ||
| } catch (e: Exception) { | ||
| // This is also fine. We don't want to bug the user. |
There was a problem hiding this comment.
you don't even want to log this?
quietly dropping exceptions sounds like a bad idea...
There was a problem hiding this comment.
We are gonna do it on separate PR as discussed above.
There was a problem hiding this comment.
created #1874 to track this
Could not reproduce #1850, but this should fix it anyway.