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.

Sign up for GitHub

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

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Increase InstrumentationResultPrinter.MAX_TRACE_SIZE #711

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...unit_runner/java/androidx/test/internal/runner/listener/InstrumentationResultPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class InstrumentationResultPrinter extends InstrumentationRunListener {

private static final String TAG = "InstrumentationResultPrinter";

@VisibleForTesting static final int MAX_TRACE_SIZE = 32 * 1024;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned this change is risky. It looks like the reason this limit exists is to prevent going over the binder IPC limit. It would seem like increasing the trace size would increase the chances of that occurring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the binder maximum size was 1 MB.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but I think it is possible that multiple stack traces will get sent in one binder transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any value here is risky, in any direction we go. Decreasing max means shorter stacktraces => Espresso is less useful. Increasing the max means more risk of hitting the binder max.

What if this didn't already have a value today, how would we pick a value? 65,536 bytes is super conservative, but 131,072 bytes is also very conservative. To send multiple stacktraces at the same time you'd need to run tests in parallel and you'd need them to fail and you'd need long stacktraces for each. Isn't that a bit far fetched?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am under the impression that multiple stack traces can get sent in one transaction even under normal conditions. Even with this trace limit in place, there are user complaints about binder size transaction errors.
#269

Long term we hope to move away from using binder to communicate test results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be hard to impose a limit on the aggregated size rather than individuals? If the cumulative stack trace size is big in one binder transaction, then stop adding more stack traces. This way, the common usefulness will increase (developer running a single test, tying to reproduce an issue and not seeing stack currently), while other cases will have more data too, but with some stacks fully missing.

Copy link
Contributor

@bohsen bohsen Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change is targeting InstrumentationResultPrinter, not OrchestrationResultPrinter/OrchestrationXmlTestRunListener which the linked issues IMO clearly shows has issues.
Also OrchestrationResultPrinter/OrchestrationXmlTestRunListener do not impose any limits on the MAX_TRACE_SIZE although the OrchestrationResultPrinter-javadoc states:

...
A line by line reimplementation of {@link
androidx.test.internal.runner.listener.InstrumentationResultPrinter}
...

(maybe they actually should)

And they have no dependency on InstrumentationResultPrinter from what I can tell.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah cool. Thanks for all the investigation everyone. There have been reports of non-orchestrator-using users hitting binder transaction issues as well, but I suspect they were making heavy use of metrics. This change sounds reasonable but I'd like to do a bit more investigation with those users. I'm pretty slammed this week but will report back next week.

@VisibleForTesting static final int MAX_TRACE_SIZE = 64 * 1024;

/**
* This value, if stored with key {@link android.app.Instrumentation#REPORT_KEY_IDENTIFIER},
Expand Down