Store entire Throwable in ErrorDetail class

[alexeyqu-fb]

There is a place at CallableImporter where we leak the exception trace into raw logs by wrapping the ErrorDetail contents in an IOException.

throw new IOException(
  "Problem with importer, forcing a retry, "
    + "first error: "
    + (errors.iterator().hasNext() ? errors.iterator().next().exception() : "none"));

This is bad for 3 reasons:

• The actual root cause of the problem is masked with a generic IOException;
• The full trace of the original exception is leaked into the exception message, bloating the logs;
• If the original exception was itself a wrapper, we'd lose the underlying cause completely.

As a first step towards fixing that, we want to modify ErrorDetail to store the Throwable instance instead of its trace in the String format.

[alexeyqu-fb]

TODO delete this

[alexeyqu-fb]

./gradlew test yields the following failure:

GoogleCloudIdempotentImportExecutorTest > removeErrorIfItemSucceeds() FAILED
    com.google.cloud.datastore.DatastoreException: The value of property "error_details" is longer than 1500 bytes.
        at com.google.cloud.datastore.spi.v1.HttpDatastoreRpc.translate(HttpDatastoreRpc.java:129)
        at com.google.cloud.datastore.spi.v1.HttpDatastoreRpc.commit(HttpDatastoreRpc.java:155)
        at com.google.cloud.datastore.DatastoreImpl$5.call(DatastoreImpl.java:418)
        at com.google.cloud.datastore.DatastoreImpl$5.call(DatastoreImpl.java:415)
        at com.google.api.gax.retrying.DirectRetryingExecutor.submit(DirectRetryingExecutor.java:105)
        at com.google.cloud.RetryHelper.run(RetryHelper.java:76)
        at com.google.cloud.RetryHelper.runWithRetries(RetryHelper.java:50)
        at com.google.cloud.datastore.DatastoreImpl.commit(DatastoreImpl.java:414)
        at com.google.cloud.datastore.TransactionImpl.commit(TransactionImpl.java:100)
        at org.datatransferproject.cloud.google.GoogleCloudIdempotentImportExecutorTest.initializeDS(GoogleCloudIdempotentImportExecutorTest.java:92)
        at org.datatransferproject.cloud.google.GoogleCloudIdempotentImportExecutorTest.removeErrorIfItemSucceeds(GoogleCloudIdempotentImportExecutorTest.java:68)

I am not sure why we need the Google Datastore dependencies, especially with the 1500 bytes limit.
It also looks like the current code will also break whenever the exception trace strings exceeds this limit.

@jzacsh @seehamrun do you think we can drop the Datastore or tweak the limits? I am not aware how it is used in the Google deployment of DTP.

[seehamrun]

I am not sure why we need the Google Datastore dependencies, especially with the 1500 bytes limit. It also looks like the current code will also break whenever the exception trace strings exceeds this limit.

Sorry for the late reply! We have the Datastore dependency because the GoogleCloud implementation of the executor is datastore based. So we use the datastore emulator in the tests for it, we cant really remove this dependency and we do use this code (or a version of it) in our internal Google implementation.

I think the limit is only applicable for the fields that are used to index, so we can mark the error_details field. something like this

StringValue.newBuilder((String) entry.getValue())
                  .setExcludeFromIndexes(true) // allows up to 1MB to be written

Whats the timeline you need this done by? We can look at modifying this for the specific field

[alexeyqu-fb]

Hi @seehamrun and thanks for the reply!

I am happy to make the change with marking the field as non-indexable.
This does not block anything specifically, but does impact the error tracking across the codebase (see my original comment for examples and reason).

Do you think it needs to be tested on Google side specifically?

[alexeyqu-fb]

Hm, maybe the best approach here is to wait for the Jackson's bugfix to serialize Throwables properly.
FasterXML/jackson-databind#1986

...which might be coming quite soon, see this discussion.