FlowCrypt / flowcrypt-android

FlowCrypt Android App
https://flowcrypt.com
Other
90 stars 11 forks source link

use shared-tenant-fes for reporting errors #2291

Closed tomholub closed 1 year ago

tomholub commented 1 year ago

https://flowcrypt.com/api/help/error should be replaced with https://flowcrypt.com/shared-tenant-fes/api/v1/log-collector/exception

The new API has the following expected JSON input format:

    val name: String, // Exception name
    val message: String, // Exception message
    val url: String, // Error url
    val line: Long, // Error line
    val col: Long, // Error line col
    val line: Int, // Error line
    val col: Int, // Error line col
    val trace: String, // Stack trace text
    val version: String, // flowcrypt version
    val environment: String, // prod/dev/unknown
    val product: String, // web-ext/android/ios/web/web-portal
    val buildType: String, // consumer/enterprise

The API will be live with FES release 2023-07 later this month.

DenBond7 commented 1 year ago

https://flowcrypt.com/api/help/error

@tomholub Do you mean this https://flowcrypt.com/api/help/acra on Android?

DenBond7 commented 1 year ago

The API will be live with FES release 2023-07 later this month.

Is it ready for testing? it looks like log-collector/exception requires authorization. What about non-com.google users?

tomholub commented 1 year ago

yes i meant the acra endpoint, in case of android

you'll have to format the details into the stack/trace field (backend used to do this, now the formatting will happen on client)

it's not available for testing yet

tomholub commented 1 year ago

yes authorisation will be needed, meaning users without id token cannot make reports

DenBond7 commented 1 year ago

@tomholub Can I continue with this task?

DenBond7 commented 1 year ago

@sosnovsky ping(just in case)

tomholub commented 1 year ago

yes, the new endpoint is up, but the new endpoint doesn't send emails, only stores errors in fb, and the dashboard to view them is not ready yet. maybe for now you could send to both, but that would be extra work? i'll leave it up to you. i'm leaving the original endpoint running until sometime in June.

DenBond7 commented 1 year ago

in that case, it will be better to leave it as is for now. I will complete this task after we will have options to review reports(via emails or web). It's not a difficult task. So migration will be fast.

DenBond7 commented 1 year ago

but the new endpoint doesn't send emails, only stores errors in fb, and the dashboard to view them is not ready yet.

@tomholub Just in case, any news here?

tomholub commented 1 year ago

we have a dashboard now, but it still doesn't send emails. i haven't had it planned yet. but from the way you were describing it, the emails were crucial

DenBond7 commented 1 year ago

but from the way you were describing it, the emails were crucial

@tomholub Do we have any progress here?

tomholub commented 1 year ago

yes, it's implemented, but not deployed. I'll deploy it this week

DenBond7 commented 1 year ago

Please notify me when it will be done. Thank you!

tomholub commented 1 year ago

it's deployed now. have you been receiving any emails?

It will email you with each new type of error on that version. Only once.

Then there will be a dashboard where you can sort errors by most commonly seen on each version.

DenBond7 commented 1 year ago

have you been receiving any emails?

No. Only usual emails from the old backend(ACRA). Android still uses https://flowcrypt.com/api/help/acra

DenBond7 commented 1 year ago

@tomholub I receive the following error for den@flowcrypt.com

{
    "code": 400,
    "message": "Domain disabled",
    "details": "For more details, see FlowCrypt External Service logs."
}
DenBond7 commented 1 year ago

I've made a request via Postman and received 200 Ok response (I've used a regular Gmail account). But I don't see any new messages for client.errors.

@tomholub Could you share CURL for https://flowcrypt.com/shared-tenant-fes/api/v1/log-collector/exception?

Also, I'd like to clarify these fields

    val url: String, // Error url
    val line: Long, // Error line
    val col: Long, // Error line col
    val line: Int, // Error line
    val col: Int, // Error line col

What information should I send here?

sosnovsky commented 1 year ago

Also, I'd like to clarify these fields

url field is appropriate only for browser extension, as there we can find on which URL exception happened, can be left null for Android line and col are where exception happened, probably not possible to be detected on Android, so should be null too.

DenBond7 commented 1 year ago

url field is appropriate only for browser extension, as there we can find on which URL exception happened, can be left null for Android

It looks like it can not be null

{
    "code": 400,
    "message": "Request body is missing the required url parameter",
    "details": "com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException: Instantiation of [simple type, class com.flowcrypt.fes.model.LogCollectorError] value failed for JSON property url due to missing (therefore NULL) value for creator parameter url which is a non-nullable type\n at [Source: (String)\"{\n    \"name\": 401,\n    \"message\": \"Authorization credentials required\",\n    \"trace\": \"io.javalin.http.UnauthorizedResponse: Authorization credentials required\",\n    \"version\": \"1.0.0\",\n    \"environment\": \"debug\",\n    \"product\": \"android\",\n    \"url\": null,\n    \"buildType\": \"dev\"\n}\"; line: 10, column: 1] (through reference chain: com.flowcrypt.fes.model.LogCollectorError[\"url\"])\n\tat com.fasterxml.jackson.module.kotlin.KotlinValueInstantiator.createFromObjectWith(KotlinValueInstantiator.kt:84)\n\tat com.fasterxml.jackson.databind.deser.impl.PropertyBasedCreator.build(PropertyBasedCreator.java:202)\n\tat com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:525)\n\tat com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1409)\n\tat com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)\n\tat com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)\n\tat com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)\n\tat com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825)\n\tat com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3772)\n\tat io.javalin.json.JavalinJackson.fromJsonString(JavalinJackson.kt:60)\n\tat com.flowcrypt.fes.server.handlers.api.logcollector.LogCollectorExceptionPost.handler(LogCollectorExceptionPost.kt:110)\n\tat com.flowcrypt.shared.server.handlers.EndpointHandler.handle(EndpointHandler.kt:43)\n\tat io.javalin.routing.HandlerEntry.handle(HandlerEntry.kt:19)\n\tat io.javalin.http.servlet.DefaultTasks.HTTP$lambda$8$lambda$6$lambda$5(DefaultTasks.kt:39)\n\tat io.javalin.http.servlet.JavalinServlet.handleTask(JavalinServlet.kt:88)\n\tat io.javalin.http.servlet.JavalinServlet.handleSync(JavalinServlet.kt:53)\n\tat io.javalin.http.servlet.JavalinServlet.service(JavalinServlet.kt:41)\n\tat jakarta.servlet.http.HttpServlet.service(HttpServlet.java:587)\n\tat io.javalin.jetty.JavalinJettyServlet.service(JavalinJettyServlet.kt:58)\n\tat jakarta.servlet.http.HttpServlet.service(HttpServlet.java:587)\n\tat org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:764)\n\tat org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:529)\n\tat org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:221)\n\tat org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1570)\n\tat org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:221)\n\tat io.javalin.jetty.JettyServer$start$wsAndHttpHandler$1.doHandle(JettyServer.kt:61)\n\tat org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:176)\n\tat org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:484)\n\tat org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1543)\n\tat org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:174)\n\tat org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1303)\n\tat org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:129)\n\tat org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:122)\n\tat org.eclipse.jetty.server.Server.handle(Server.java:563)\n\tat org.eclipse.jetty.server.HttpChannel.lambda$handle$0(HttpChannel.java:505)\n\tat org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:762)\n\tat org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:497)\n\tat org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:282)\n\tat org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:314)\n\tat org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)\n\tat org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)\n\tat org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:416)\n\tat org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:385)\n\tat org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:272)\n\tat org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.lambda$new$0(AdaptiveExecutionStrategy.java:140)\n\tat org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:411)\n\tat org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:969)\n\tat org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1194)\n\tat org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1149)\n\tat java.base/java.lang.Thread.run(Thread.java:833)\n"
}
sosnovsky commented 1 year ago

url field is appropriate only for browser extension, as there we can find on which URL exception happened, can be left null for Android

It looks like it can not be null

Yeah, we've missed it, I filed an issue to make url optional - https://github.com/FlowCrypt/enterprise-server/issues/5185 For now, we can just set it to android for Android exceptions

DenBond7 commented 1 year ago

@sosnovsky Can you check does a server receive a report and send a message to the mailbox?

sosnovsky commented 1 year ago

@sosnovsky Can you check does a server receive a report and send a message to the mailbox?

Unfortunately no, I think only @tomholub has access to reports dashboard and receives mails about new exceptions

DenBond7 commented 1 year ago

got it, thank you. I will wait for Tom's reply.

tomholub commented 1 year ago

wait, doesnt android have some view path? put that in url

DenBond7 commented 1 year ago

wait, doesnt android have some view path?

Not sure. Could you clarify what do you mean?

DenBond7 commented 1 year ago

For now I receive the success response from a server. Buy I don't receive any reports to my mailbox.

tomholub commented 1 year ago

wait, doesnt android have some view path?

Not sure. Could you clarify what do you mean?

I had an impression, when you run tests, you start the view (or navigate views) by some sort of paths or urls. But I may be wrong.

Well, you could just put the name of the view where the error occured? That sounds helpful enough. But if it's not important for you, then just add "-" in there. There's no need to edit the ES code, so I closed that issue.

tomholub commented 1 year ago

Hm. No emails. I'll check the configuration.

DenBond7 commented 1 year ago

It will email you with each new type of error on that version. Only once.

@tomholub I have a question. It's nice that we will not receive a lot of repeated notifications. But it would be nice to receive a new repeated notification for every 50 occurrences (less or more. can be discussed). How I see it:

  1. we receive a report with name = "report". 1 occurrence. MAKE A NOTIFICATION
  2. we receive a report with name = "report". 2 occurrence. Do not notify
  3. we receive a report with name = "report". 3 occurrence. Do not notify. And so on.
  4. we receive a report with name = "report". 51 occurrence. MAKE A NOTIFICATION
  5. we receive a report with name = "report". 52 occurrence. Do not notify
  6. we receive a report with name = "report". 53 occurrence. Do not notify. And so on.
  7. we receive a report with name = "report". 101 occurrence. MAKE A NOTIFICATION. And so on.

It will help cover a case when we will have an urgent issue. For example, we make a new release, and users are became receiving crashes in one place. It will produce a lot of the same reports and to handle this case I will have to monitor the dashboard.

DenBond7 commented 1 year ago

I have a question. It's nice that we will not receive a lot of repeated notifications. But it would be nice to receive a new repeated notification for every 50 occurrences (less or more. can be discussed). How I see it:

I've created a separate issue in https://github.com/FlowCrypt/enterprise-server