feat(submission): handle unparseable reports gracefully and improve atomic file writes#1472
Conversation
…tomic file writes
sdsantos
left a comment
sdsantos
left a comment
There was a problem hiding this comment.
I'm wondering if we could use an easier approach for unparsable reports. Why not just mark those measurements as is_failed=true? I know the engine did not report them as failed, but if we end up with an unparsable report, it's like it failed anyway, we can't do nothing with it. And that way it would appear to users as a measurement that failed to upload, and they can do nothing about it.
|
|
||
| // Defensive: delete any orphaned `.tmp` siblings left by prior killed writes. | ||
| parent?.let { dir -> | ||
| runCatching { |
There was a problem hiding this comment.
Maybe we don't care about logging errors here very much, but I'm wondering if we shouldn't logging them anyway...
| .use { sink -> | ||
| sink.buffer().use { | ||
| it.writeUtf8(contents) | ||
| if (append) { |
There was a problem hiding this comment.
This makes me think if we shouldn't have another AppendFile class just for the append use case, since it's so different now.
| } | ||
| } | ||
|
|
||
| Instrumentation.reportTransaction( |
There was a problem hiding this comment.
This will mean 1 transaction every 5 seconds on every device, due to the logs write. Not sure we want that many transactions going on...
| fileSystem.atomicMove(tmp, absolutePath) | ||
| } catch (e: IOException) { | ||
| // Never regress to "cannot write at all" if a filesystem can't do an atomic move. | ||
| Logger.w("Atomic write failed for $path, falling back to direct write", e) |
There was a problem hiding this comment.
Do all platforms support atomic moves? Otherwise this warning log will get sent every 5 seconds on those platforms.
| WHERE Measurement.is_done = 1 | ||
| AND Measurement.is_uploaded = 0 | ||
| AND (:filterByResultId = 0 OR Measurement.result_id = :resultId) | ||
| AND (Measurement.is_upload_failed = 0 OR Measurement.upload_failure_msg IS NULL OR Measurement.upload_failure_msg NOT LIKE 'Report unparseable:%') |
There was a problem hiding this comment.
This query isn't used much, so it doesn't matter to have this extra condition here.
But I don't like that Report unparseable here is hardcoded, and needs to match the other constant in the code. Plus this is very easy to filter in memory.
Not a big deal, we can leave it as it is. But I wanted to give it an heads up.
There was a problem hiding this comment.
i get the concern, is_failed would be better
2 participants
Closes #1451