-
Notifications
You must be signed in to change notification settings - Fork 1k
add slow request sampler #4978
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.
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
base: main
Are you sure you want to change the base?
add slow request sampler #4978
Changes from all commits
1ffe771
229901f
0a0bc05
5a23443
01847b8
a85b8f8
7222a1c
b17cef3
a26c7be
28ba89d
16fd779
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /* | ||
| * Copyright 2023 LINE Corporation | ||
| * | ||
| * LINE Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| */ | ||
| package com.linecorp.armeria.common.util; | ||
|
|
||
| /** | ||
| * Sample if both of the samplers sample. | ||
| */ | ||
| final class AndSampler<T> implements Sampler<T> { | ||
|
|
||
| private final Sampler<T> left; | ||
| private final Sampler<T> right; | ||
|
|
||
| AndSampler(Sampler<T> left, Sampler<T> right) { | ||
| this.left = left; | ||
| this.right = right; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isSampled(T t) { | ||
| // Assign the variables otherwise the short-circuiting will cause sampler to not be used. | ||
| final boolean leftSampled = left.isSampled(t); | ||
| final boolean rightSampled = right.isSampled(t); | ||
| return leftSampled && rightSampled; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return left + " and " + right; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /* | ||
| * Copyright 2023 LINE Corporation | ||
| * | ||
| * LINE Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| */ | ||
| package com.linecorp.armeria.common.util; | ||
|
|
||
| /** | ||
| * Sample if one of the samplers samples. | ||
| */ | ||
| final class OrSampler<T> implements Sampler<T> { | ||
|
|
||
| private final Sampler<T> left; | ||
| private final Sampler<T> right; | ||
|
|
||
| OrSampler(Sampler<T> left, Sampler<T> right) { | ||
| this.left = left; | ||
| this.right = right; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isSampled(T t) { | ||
| // Assign the variables otherwise the short-circuiting will cause sampler to not be used. | ||
| final boolean leftSampled = left.isSampled(t); | ||
| final boolean rightSampled = right.isSampled(t); | ||
| return leftSampled || rightSampled; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return left + " or " + right; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,107 @@ | ||||||
| /* | ||||||
| * Copyright 2023 LINE Corporation | ||||||
| * | ||||||
| * LINE Corporation licenses this file to you under the Apache License, | ||||||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||||||
| * with the License. You may obtain a copy of the License at: | ||||||
| * | ||||||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||||||
| */ | ||||||
| package com.linecorp.armeria.common.util; | ||||||
|
|
||||||
| import java.time.Duration; | ||||||
| import java.util.concurrent.TimeUnit; | ||||||
| import java.util.concurrent.atomic.AtomicReference; | ||||||
|
|
||||||
| import com.google.common.annotations.VisibleForTesting; | ||||||
| import com.google.common.base.MoreObjects; | ||||||
|
|
||||||
| import com.linecorp.armeria.common.metric.MoreMeters; | ||||||
|
|
||||||
| import io.micrometer.core.instrument.Clock; | ||||||
| import io.micrometer.core.instrument.distribution.DistributionStatisticConfig; | ||||||
| import io.micrometer.core.instrument.distribution.HistogramSnapshot; | ||||||
| import io.micrometer.core.instrument.distribution.TimeWindowPercentileHistogram; | ||||||
|
|
||||||
| /** | ||||||
| * Sample if the value is less than the percentile of the values in the last window. | ||||||
| */ | ||||||
| final class TimeWindowPercentileSampler implements Sampler<Long> { | ||||||
|
|
||||||
| private final float percentile; | ||||||
| private final long windowLengthMillis; | ||||||
| private final TimeWindowPercentileHistogram histogram; | ||||||
| private final long snapshotUpdateNanos; | ||||||
| private static final long DEFAULT_SNAPSHOT_UPDATE_NANOS = TimeUnit.SECONDS.toNanos(1); | ||||||
| private long lastSnapshotNanos; | ||||||
| private HistogramSnapshot histogramSnapshot; | ||||||
|
|
||||||
| private final Clock clock; | ||||||
| private final AtomicReference<Boolean> isTakingSnapshot = new AtomicReference<>(false); | ||||||
|
|
||||||
| TimeWindowPercentileSampler(float percentile, long windowLengthMillis) { | ||||||
| this(percentile, windowLengthMillis, Clock.SYSTEM, DEFAULT_SNAPSHOT_UPDATE_NANOS); | ||||||
| } | ||||||
|
|
||||||
| @VisibleForTesting | ||||||
| TimeWindowPercentileSampler(float percentile, long windowLengthMillis, Clock clock, | ||||||
| long snapshotUpdateNanos) { | ||||||
| this.percentile = percentile; | ||||||
| this.windowLengthMillis = windowLengthMillis; | ||||||
|
|
||||||
| final DistributionStatisticConfig distributionStatisticConfig = | ||||||
| DistributionStatisticConfig.builder() | ||||||
| .percentilesHistogram(false) | ||||||
| .percentiles(percentile) | ||||||
| .expiry(Duration.ofMillis(windowLengthMillis)) | ||||||
| .build() | ||||||
| .merge(MoreMeters.distributionStatisticConfig()); | ||||||
| this.histogram = new TimeWindowPercentileHistogram(clock, distributionStatisticConfig, true); | ||||||
| this.snapshotUpdateNanos = snapshotUpdateNanos; | ||||||
| this.histogramSnapshot = histogram.takeSnapshot(0, 0, 0); | ||||||
| this.clock = clock; | ||||||
| this.lastSnapshotNanos = clock.monotonicTime(); | ||||||
| } | ||||||
|
|
||||||
| @VisibleForTesting | ||||||
| static TimeWindowPercentileSampler create(float percentile, long windowLengthMillis, | ||||||
| long snapshotUpdateNanos) { | ||||||
| return new TimeWindowPercentileSampler(percentile, windowLengthMillis, Clock.SYSTEM, | ||||||
| snapshotUpdateNanos); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public boolean isSampled(Long t) { | ||||||
| histogram.recordLong(t); | ||||||
|
|
||||||
| System.out.println("lastSnapshotNanos: " + lastSnapshotNanos); | ||||||
| System.out.println("snapshotUpdateNanos: " + snapshotUpdateNanos); | ||||||
| System.out.println("clock.monotonicTime(): " + clock.monotonicTime()); | ||||||
|
|
||||||
| if (lastSnapshotNanos + snapshotUpdateNanos <= clock.monotonicTime()) { | ||||||
| if (isTakingSnapshot.compareAndSet(false, true)) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we implement a double-checking pattern for the update?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, I am just duplicating the if condition in L70 to L72 to double check. Does it sound good? |
||||||
| // Two threads reach here back to back. Make sure snapshot is not taken very recently before | ||||||
| // we acquired the lock. | ||||||
| if (lastSnapshotNanos + snapshotUpdateNanos <= clock.monotonicTime()) { | ||||||
| System.out.println("Taking snapshot"); | ||||||
| histogramSnapshot = histogram.takeSnapshot(0, 0, 0); | ||||||
| lastSnapshotNanos = clock.monotonicTime(); | ||||||
| isTakingSnapshot.set(false); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| final Double percentileValue = histogramSnapshot.percentileValues()[0].value(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, Kotlin habits 😆
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm turns out it doesn't work this way. Unit tests just started failing. I believe .value() just returns an object thus I need it this way. Or this needs to change I don't know why it would it fail anyway.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, it just rounds down with |
||||||
| return t >= percentileValue.longValue(); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public String toString() { | ||||||
| return MoreObjects.toStringHelper(this) | ||||||
| .omitNullValues() | ||||||
| .add("percentile", percentile) | ||||||
| .add("windowLengthMillis", windowLengthMillis) | ||||||
| .add("snapshotUpdateNanos", snapshotUpdateNanos) | ||||||
| .toString(); | ||||||
|
Check warning on line 105 in core/src/main/java/com/linecorp/armeria/common/util/TimeWindowPercentileSampler.java
|
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,17 +61,23 @@ public static LoggingServiceBuilder builder() { | |||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| LoggingService(HttpService delegate, LogWriter logWriter, | ||||||||||||||||||||||||||||||||||||||||||
| Sampler<? super ServiceRequestContext> successSampler, | ||||||||||||||||||||||||||||||||||||||||||
| Sampler<? super ServiceRequestContext> failureSampler) { | ||||||||||||||||||||||||||||||||||||||||||
| Sampler<? super ServiceRequestContext> failureSampler, | ||||||||||||||||||||||||||||||||||||||||||
| Sampler<Long> slowRequestSampler) { | ||||||||||||||||||||||||||||||||||||||||||
| super(requireNonNull(delegate, "delegate")); | ||||||||||||||||||||||||||||||||||||||||||
| this.logWriter = requireNonNull(logWriter, "logWriter"); | ||||||||||||||||||||||||||||||||||||||||||
| requireNonNull(successSampler, "successSampler"); | ||||||||||||||||||||||||||||||||||||||||||
| requireNonNull(failureSampler, "failureSampler"); | ||||||||||||||||||||||||||||||||||||||||||
| sampler = requestLog -> { | ||||||||||||||||||||||||||||||||||||||||||
| final ServiceRequestContext ctx = (ServiceRequestContext) requestLog.context(); | ||||||||||||||||||||||||||||||||||||||||||
| final boolean isSlow = slowRequestSampler.isSampled(requestLog.totalDurationNanos()); | ||||||||||||||||||||||||||||||||||||||||||
| final boolean successOrFailure; | ||||||||||||||||||||||||||||||||||||||||||
| if (ctx.config().successFunction().isSuccess(ctx, requestLog)) { | ||||||||||||||||||||||||||||||||||||||||||
| return successSampler.isSampled(ctx); | ||||||||||||||||||||||||||||||||||||||||||
| successOrFailure = successSampler.isSampled(ctx); | ||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||
| successOrFailure = failureSampler.isSampled(ctx); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return failureSampler.isSampled(ctx); | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+72
to
-74
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Samplers are stateful, if we short cut, it would mean sampler won't record the value. I.e. counting sampler won't count actual values.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are tests that verify this there is similar behavior here? armeria/core/src/test/java/com/linecorp/armeria/common/util/SamplerTest.java Lines 139 to 158 in 01847b8
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return successOrFailure || isSlow; | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.