Use `Sampler` to determine whether to fill stack trace #2111

trustin · 2019-09-25T06:23:17Z

Motivation:

Flags.verboseExceptions() gives a user only two choices - enabling
stacktrace for all exceptions or not. We could let a user take
trade-offs by allowing him or her to specify a Sampler specification.

Modifications:

Accept a Sampler specification string as a value of the
com.linecorp.armeria.verboseExceptions system property.
- Use the default specification of rate-limited=10, which allows 10
  traces per second per exception type.
- Note that true and false are translated into always and
  never.
Add ExceptionSampler
Replace Flags.verboseExceptions() with verboseExceptionSampler()
and verboseExceptionSamplerSpec()
The exceptions which never had stack traces can now have stack trace.
Add a benchmark

Result:

A user will get the stack trace for some exceptions out of the box,
which is a better experience.
- Previously, a user had to set true to the flag manually.
A user can trade-off between the cost of filling in stack trace and
the debuggability in a fine-grained manner.

Benchmark result:

Benchmark                                              Mode  Cnt          Score          Error  Units
ExceptionSamplerBenchmark.baseline_singleton          thrpt    5  115913078.321 ±  2470676.668  ops/s
ExceptionSamplerBenchmark.baseline_withStackTrace     thrpt    5     435184.216 ±    68777.567  ops/s
ExceptionSamplerBenchmark.baseline_withoutStackTrace  thrpt    5   51507410.813 ±  4643400.389  ops/s
ExceptionSamplerBenchmark.sampler_always              thrpt    5     460633.461 ±    23792.691  ops/s
ExceptionSamplerBenchmark.sampler_never               thrpt    5   65191830.351 ± 10603815.026  ops/s
ExceptionSamplerBenchmark.sampler_random_1            thrpt    5   22901381.973 ±   992182.987  ops/s
ExceptionSamplerBenchmark.sampler_random_10           thrpt    5    4131608.570 ±    62347.977  ops/s
ExceptionSamplerBenchmark.sampler_rateLimited_1       thrpt    5   16322277.831 ±   591082.048  ops/s
ExceptionSamplerBenchmark.sampler_rateLimited_10      thrpt    5   15205268.392 ±   339038.406  ops/s

Motivation: `Flags.verboseExceptions()` gives a user only two choices - enabling stacktrace for all exceptions or not. We could let a user take trade-offs by allowing him or her to specify a `Sampler` specification. Modifications: - Accept a `Sampler` specification string as a value of the `com.linecorp.armeria.verboseExceptions` system property. - Use the default specification of `rate-limited=1`, which allows one trace per second per exception type. - Note that `true` and `false` are translated into `always` and `never`. - Add `ExceptionSampler` - Replace Flags.verboseExceptions()` with `verboseExceptionSampler()` and `verboseExceptionSamplerSpec()` - Add a benchmark Result: - A user will get the stack trace for some exceptions out of the box, which is a better experience. - Previously, a user had to set `true` to the flag manually. - A user can trade-off between the cost of filling in stack trace and the debuggability in a fine-grained manner. Benchmark result: ``` Benchmark Mode Cnt Score Error Units ExceptionSamplerBenchmark.baseline_singleton thrpt 5 113645503.499 ± 4733673.802 ops/s ExceptionSamplerBenchmark.baseline_withStackTrace thrpt 5 415370.775 ± 117032.077 ops/s ExceptionSamplerBenchmark.baseline_withoutStackTrace thrpt 5 50683298.670 ± 8876147.874 ops/s ExceptionSamplerBenchmark.sampler_always thrpt 5 423056.060 ± 81934.927 ops/s ExceptionSamplerBenchmark.sampler_never thrpt 5 67165540.355 ± 13892391.328 ops/s ExceptionSamplerBenchmark.sampler_rateLimited thrpt 5 15781563.939 ± 1037630.515 ops/s ```

trustin · 2019-09-25T06:25:08Z

@adriancole You might find this interesting.

minwoox · 2019-09-25T06:26:07Z

~~What is the rate that you used in the benchmark test?~~
Ah I found that it's 1 for second. Thanks!

benchmarks/src/jmh/java/com/linecorp/armeria/common/ExceptionSamplerBenchmark.java

core/src/main/java/com/linecorp/armeria/common/ExceptionSampler.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

minwoox · 2019-09-25T06:45:28Z

core/src/main/java/com/linecorp/armeria/common/ExceptionSampler.java

+    private final String spec;
+
+    ExceptionSampler(String spec) {
+        samplerFactory = unused -> Sampler.of(spec);


Is there any chance that a user wants to set different specs for different exceptions?

I'm not sure. Perhaps we can consider that when someone asks for it? 😉

Added a comment about this idea to Flags

codefromthecrypt · 2019-09-25T08:00:08Z

core/src/main/java/com/linecorp/armeria/common/ExceptionSampler.java

+    }
+
+    @Override
+    public boolean isSampled(Class<? extends Throwable> exceptionType) {


so the rationale for the type parameter is solving the overhead problem caused by allocating an exception, correct?

Yeah, kind of. If we accepted a Throwable, we will see a chicken and egg problem. 😄

minwoox

Nice work! 👍

trustin · 2019-09-25T08:23:40Z

sampler_rateLimited_10 was tainted by wrong input. Here's the correct one:

Benchmark                                              Mode  Cnt          Score          Error  Units
ExceptionSamplerBenchmark.baseline_singleton          thrpt    5  115913078.321 ±  2470676.668  ops/s
ExceptionSamplerBenchmark.baseline_withStackTrace     thrpt    5     435184.216 ±    68777.567  ops/s
ExceptionSamplerBenchmark.baseline_withoutStackTrace  thrpt    5   51507410.813 ±  4643400.389  ops/s
ExceptionSamplerBenchmark.sampler_always              thrpt    5     460633.461 ±    23792.691  ops/s
ExceptionSamplerBenchmark.sampler_never               thrpt    5   65191830.351 ± 10603815.026  ops/s
ExceptionSamplerBenchmark.sampler_random_1            thrpt    5   22901381.973 ±   992182.987  ops/s
ExceptionSamplerBenchmark.sampler_random_10           thrpt    5    4131608.570 ±    62347.977  ops/s
ExceptionSamplerBenchmark.sampler_rateLimited_1       thrpt    5   16322277.831 ±   591082.048  ops/s
ExceptionSamplerBenchmark.sampler_rateLimited_10      thrpt    5   15205268.392 ±   339038.406  ops/s

ikhoon

👍

codecov · 2019-09-25T09:14:10Z

Codecov Report

Merging #2111 into master will decrease coverage by 0.08%.
The diff coverage is 41.66%.

@@             Coverage Diff              @@
##             master    #2111      +/-   ##
============================================
- Coverage     73.78%   73.69%   -0.09%     
+ Complexity     9491     9488       -3     
============================================
  Files           833      834       +1     
  Lines         36545    36585      +40     
  Branches       4525     4527       +2     
============================================
- Hits          26964    26963       -1     
- Misses         7253     7292      +39     
- Partials       2328     2330       +2

Impacted Files	Coverage Δ	Complexity Δ
...a/common/grpc/protocol/ArmeriaStatusException.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...a/com/linecorp/armeria/common/util/Exceptions.java	`39.47% <ø> (+0.34%)`	`25 <0> (ø)`	⬇️
.../com/linecorp/armeria/common/TimeoutException.java	`33.33% <ø> (+13.33%)`	`2 <0> (+1)`	⬆️
...orp/armeria/common/ProtocolViolationException.java	`33.33% <0%> (ø)`	`2 <1> (ø)`	⬇️
...om/linecorp/armeria/server/saml/SamlException.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...inecorp/armeria/client/RefusedStreamException.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...m/linecorp/armeria/server/HttpStatusException.java	`57.89% <0%> (ø)`	`6 <0> (ø)`	⬇️
.../com/linecorp/armeria/common/ExceptionSampler.java	`0% <0%> (ø)`	`0 <0> (?)`
...rp/armeria/client/UnprocessedRequestException.java	`62.5% <0%> (ø)`	`3 <1> (ø)`	⬇️
...linecorp/armeria/server/HttpResponseException.java	`75% <0%> (ø)`	`6 <1> (ø)`	⬇️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e8f016...aabbf96. Read the comment docs.

minwoox · 2019-09-25T09:16:40Z

...protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/ArmeriaStatusException.java

@@ -23,7 +23,7 @@
 */
 public class ArmeriaStatusException extends RuntimeException {

-    public static final long serialVersionUID = -8370257107063108923L;
+    private static final long serialVersionUID = -8370257107063108923L;


minwoox · 2019-09-25T09:21:06Z

still LGTM 👍

ikhoon · 2019-09-25T09:39:24Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

@@ -95,16 +95,19 @@

        switch (spec) {
            case "true":
+            case "always":


Motivation: `Flags.verboseExceptions()` gives a user only two choices - enabling stacktrace for all exceptions or not. We could let a user take trade-offs by allowing him or her to specify a `Sampler` specification. Modifications: - Accept a `Sampler` specification string as a value of the `com.linecorp.armeria.verboseExceptions` system property. - Use the default specification of `rate-limited=10`, which allows 10 traces per second per exception type. - Note that `true` and `false` are translated into `always` and `never`. - Add `ExceptionSampler` - Replace `Flags.verboseExceptions()` with `verboseExceptionSampler()` and `verboseExceptionSamplerSpec()` - The exceptions which never had stack traces can now have stack trace. - Add a benchmark Result: - A user will get the stack trace for some exceptions out of the box, which is a better experience. - Previously, a user had to set `true` to the flag manually. - A user can trade-off between the cost of filling in stack trace and the debuggability in a fine-grained manner. Benchmark result: ``` Benchmark Mode Cnt Score Error Units ExceptionSamplerBenchmark.baseline_singleton thrpt 5 115913078.321 ± 2470676.668 ops/s ExceptionSamplerBenchmark.baseline_withStackTrace thrpt 5 435184.216 ± 68777.567 ops/s ExceptionSamplerBenchmark.baseline_withoutStackTrace thrpt 5 51507410.813 ± 4643400.389 ops/s ExceptionSamplerBenchmark.sampler_always thrpt 5 460633.461 ± 23792.691 ops/s ExceptionSamplerBenchmark.sampler_never thrpt 5 65191830.351 ± 10603815.026 ops/s ExceptionSamplerBenchmark.sampler_random_1 thrpt 5 22901381.973 ± 992182.987 ops/s ExceptionSamplerBenchmark.sampler_random_10 thrpt 5 4131608.570 ± 62347.977 ops/s ExceptionSamplerBenchmark.sampler_rateLimited_1 thrpt 5 16322277.831 ± 591082.048 ops/s ExceptionSamplerBenchmark.sampler_rateLimited_10 thrpt 5 15205268.392 ± 339038.406 ops/s ```

trustin added the new feature label Sep 25, 2019

trustin added this to the 0.93.0 milestone Sep 25, 2019

trustin requested review from anuraaga, ikhoon and minwoox as code owners September 25, 2019 06:23

More benchmarks

2bc6273

trustin force-pushed the exception_sampling branch from 89c9d4c to 2bc6273 Compare September 25, 2019 06:40

Checkstyle

c5091ac

minwoox reviewed Sep 25, 2019

View reviewed changes

trustin added 5 commits September 25, 2019 16:05

Address the comments from @minwoox

512b558

Use the right spec

523bc76

Update Javadoc

a9be99c

Add XXX

c94da43

Add missing finals

2a32e70

codefromthecrypt reviewed Sep 25, 2019

View reviewed changes

Fix benchmark

91f7bec

minwoox added the breaking change label Sep 25, 2019

minwoox approved these changes Sep 25, 2019

View reviewed changes

Checkstyle

c7af530

ikhoon approved these changes Sep 25, 2019

View reviewed changes

trustin force-pushed the exception_sampling branch from 16ff042 to c7af530 Compare September 25, 2019 08:27

Fill stack traces for more exception types

d1b6455

minwoox reviewed Sep 25, 2019

View reviewed changes

Optimize a little bit

aabbf96

ikhoon reviewed Sep 25, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/Flags.java

@@ -95,16 +95,19 @@

switch (spec) {

case "true":

case "always":

Copy link

Contributor

ikhoon Sep 25, 2019

Choose a reason for hiding this comment

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

👍

trustin merged commit 63b9e42 into line:master Sep 25, 2019

trustin deleted the exception_sampling branch September 25, 2019 10:56

trustin mentioned this pull request Sep 25, 2019

Add sampling to the exceptions without stack traces. #2105

Closed

Use Sampler to determine whether to fill stack trace #2111

Use Sampler to determine whether to fill stack trace #2111

Uh oh!

Conversation

trustin commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trustin commented Sep 25, 2019

Uh oh!

minwoox commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minwoox Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

trustin Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

trustin Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

trustin Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

trustin commented Sep 25, 2019

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

minwoox Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

minwoox commented Sep 25, 2019

Uh oh!

ikhoon Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Use `Sampler` to determine whether to fill stack trace #2111

Use `Sampler` to determine whether to fill stack trace #2111

trustin commented Sep 25, 2019 •

edited

Loading

minwoox commented Sep 25, 2019 •

edited

Loading

codecov bot commented Sep 25, 2019 •

edited

Loading