Skip to content
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.toString() + " and " + right.toString();
Comment thread
Dogacel marked this conversation as resolved.
Outdated
}
}
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.toString() + " or " + right.toString();
Comment thread
Dogacel marked this conversation as resolved.
Outdated
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,64 @@
*/
@FunctionalInterface
public interface Sampler<T> {

/**
* Returns a sampler that applies logical or operator to both samplers decisions.
*/
default Sampler<T> or(Sampler<T> other) {
Comment thread
Dogacel marked this conversation as resolved.
return new OrSampler<>(this, other);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return new OrSampler<>(this, other);
return new OrSampler<>(this, requireNonNull(other, "other"));

Can we also consider the situation where more than two samplers are chained? e.g. a.or(b).or(c) by accepting an array of samplers in OrSampler.<init>()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we accept an array of samplers, a new syntax can be used a.or(b, c, d). Is this syntax something we want?

Because .or returns another sampler, we can safely call another .or as you described.

a.or(b).or(c) looks better than a.or(b, c) in my opinion, what do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I was actually talking about the constructor of OrSampler(). or() should accept only one parameter. OrSampler.or() could be optimized so that OrSampler doesn't wrap another OrSampler.

}

/**
* Returns a sampler that applies logical and operator to both samplers decisions.
*/
default Sampler<T> and(Sampler<T> other) {
return new AndSampler<>(this, other);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return new AndSampler<>(this, other);
return new AndSampler<>(this, requireNonNull(other));

Can we also consider the situation where more than two samplers are chained? e.g. a.and(b).and(c) by accepting an array of samplers in AndSampler.<init>()?

Should we add a static version of and() and or()? (not sure if this is the best idea though. let me know what you think.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did not like how the static function looks syntactically. Because there are no extension functions in java we need to call them such as Sampler.and(a, b) instead of a.and(b) right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right. I'm fine with not adding a static method. I just wish And/OrSampler doesn't wrap another And/OrSampler, which is suboptimal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. I am not sure if I am 100% following. Why wrapping another sampler is not optimal?

If we are talking about "short circuiting" the evaluation, that's something we don't want to do.

}

/**
* Returns a sampler that returns {@code true} if the value is greater than the given value.
*/
static <T extends Comparable<T>> Sampler<T> greaterThan(T val) {
return object -> object.compareTo(val) > 0;
Comment thread
Dogacel marked this conversation as resolved.
}

/**
* Returns a sampler that returns {@code true} if the value is less than or equal to the given value.
*/
static <T extends Comparable<T>> Sampler<T> greaterThanOrEqual(T val) {
return object -> object.compareTo(val) >= 0;
}

/**
* Returns a sampler that returns {@code true} if the value is less than the given value.
*/
static <T extends Comparable<T>> Sampler<T> lessThan(T val) {
return object -> object.compareTo(val) < 0;
}

/**
* Returns a sampler that returns {@code true} if the value is less than or equal to the given value.
*/
static <T extends Comparable<T>> Sampler<T> lessThanOrEqual(T val) {
return object -> object.compareTo(val) <= 0;
}

/**
* Returns a sampler that returns {@code true} if the value is equal to the given value.
*/
static <T extends Comparable<T>> Sampler<T> equal(T val) {
return object -> object.compareTo(val) == 0;
}

/**
* Returns a sampler that returns {@code true} if the value is inside the percentile distribution
* in the given time window.
*/
static Sampler<Long> percentile(float percentile, long windowMilliseconds) {
return new TimeWindowPercentileSampler(percentile, windowMilliseconds);
}

/**
* Returns a probabilistic sampler which samples at the specified {@code probability}
* between {@code 0.0} and {@code 1.0}.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* 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 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 static final long SNAPSHOT_UPDATE_MILLIS = 1000L;
private long lastSnapshotMillis = 0L;
private HistogramSnapshot histogramSnapshot;

TimeWindowPercentileSampler(float percentile, long windowLengthMillis) {
this.percentile = percentile;
this.windowLengthMillis = windowLengthMillis;

final DistributionStatisticConfig distributionStatisticConfig =
DistributionStatisticConfig.builder()
.percentiles(percentile)
.percentilePrecision(2)
.minimumExpectedValue(1.0)
.maximumExpectedValue(Double.POSITIVE_INFINITY)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we also make these three properties configurable? They are important for trading off between memory and accuracy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should we expose those variables to the user? Or do we want to make them system properties?

Because we have so many things here, I tried to simplify as much as possible to keep LoggingDecorator simple. So, do system properties or flags make more sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah I have one idea, let's re-use MoreMeters.

.merge(MoreMeters.distributionStatisticConfig())

I will only give percentiles and expiry policy. Rest should be shared configuration by default.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

.expiry(Duration.ofMillis(windowLengthMillis))
.bufferLength(3)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also should be configurable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe the following helps with configuration

.merge(MoreMeters.distributionStatisticConfig());

So basically we are using the same buffer length we use for distribution statistics, which makes sense on my side. Any thoughts?

.build();
this.histogram = new TimeWindowPercentileHistogram(Clock.SYSTEM, distributionStatisticConfig, true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should make the Clock injectable with @VisisbleForTesting to test it properly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a test-only constructor that takes a clock 👍

this.histogramSnapshot = histogram.takeSnapshot(0, 0, 0);
this.lastSnapshotMillis = System.currentTimeMillis();
}

static TimeWindowPercentileSampler create(float percentile, long windowLengthMillis) {
Comment thread
Dogacel marked this conversation as resolved.
Outdated
return new TimeWindowPercentileSampler(percentile, windowLengthMillis);
}

@Override
public boolean isSampled(Long t) {
histogram.recordLong(t);

if (lastSnapshotMillis + SNAPSHOT_UPDATE_MILLIS < System.currentTimeMillis()) {
histogramSnapshot = histogram.takeSnapshot(0, 0, 0);
lastSnapshotMillis = System.currentTimeMillis();
}
Comment thread
Dogacel marked this conversation as resolved.
Outdated

final Double percentileValue = histogramSnapshot.percentileValues()[0].value();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
final Double percentileValue = histogramSnapshot.percentileValues()[0].value();
final double percentileValue = histogramSnapshot.percentileValues()[0].value();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops, Kotlin habits 😆

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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

        return t >= percentileValue.longValue();

I don't know why it would it fail anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see, it just rounds down with .longValue() which works better in my case because I don't really care about the decimal points. This causes maximum value to be never sampled because it is just an approximation 🤔

return percentileValue.longValue() <= t;
Comment thread
Dogacel marked this conversation as resolved.
Outdated
}

@Override
public String toString() {
return "SlidingWindowPercentileSampler with " + windowLengthMillis + " ms window and " + percentile +
Comment thread
Dogacel marked this conversation as resolved.
Outdated
" percentile";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,28 @@
*/
float failureSamplingRate() default -1.0f;

/**
* Sample the requests if they are slower than the {@code slowRequestSamplingPercentile} percent
* of the requests.
*/
float slowRequestSamplingPercentile() default -1.0f;

/**
* Slow request percentiles are calculated over the last {@code slowRequestSamplingWindowMilliseconds}.
*/
long slowRequestSamplingWindowMilliseconds() default 10 * 60 * 1000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We prefer Millis to Milliseconds

Suggested change
long slowRequestSamplingWindowMilliseconds() default 10 * 60 * 1000;
long slowRequestSamplingWindowMillis() default 10 * 60 * 1000;


/**
* Don't sample the requests if they are faster than the {@code slowRequestSamplingLowerBoundMilliseconds}.
* Should be used with {@link #slowRequestSamplingUpperBoundMilliseconds()}.
*/
long slowRequestSamplingLowerBoundMilliseconds() default 0L;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
long slowRequestSamplingLowerBoundMilliseconds() default 0L;
long slowRequestSamplingLowerBoundMillis() default 0L;


/**
* Always sample the requests if they are slower than the {@code slowRequestSamplingUpperBoundMilliseconds}.
*/
long slowRequestSamplingUpperBoundMilliseconds() default Long.MAX_VALUE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
long slowRequestSamplingUpperBoundMilliseconds() default Long.MAX_VALUE;
long slowRequestSamplingUpperBoundMillis() default Long.MAX_VALUE;


Comment on lines +86 to +107

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@trustin @ikhoon @minwoox

I would like to continue working on this if you find any value on this. I find value on adding this so there is a good observability tool coming out-of-the box with Armeria, similar to printing failures with this decorator.

For example in my company, we implemented a method which has an hardcoded upper-bound only to capture slow requests right now to work-around this.

If we would like to discuss the interface we should provide to achieve this, I am open to it. I know this might not be any priority for Armeria but I would be happy to hear from you when you are available 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally think the existing values look sensible which are disabled by default and set values to enable the feature.

/**
* The order of decoration, where a {@link Decorator} of lower value will be applied first.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.annotation.DecoratorFactoryFunction;
import com.linecorp.armeria.server.logging.LoggingService;
import com.linecorp.armeria.server.logging.LoggingServiceBuilder;

/**
* A factory which creates a {@link LoggingService} decorator.
Expand All @@ -38,15 +39,23 @@ public final class LoggingDecoratorFactoryFunction implements DecoratorFactoryFu
final float failureSamplingRate =
parameter.failureSamplingRate() >= 0.0f ? parameter.failureSamplingRate()
: parameter.samplingRate();
return LoggingService.builder()
.logWriter(LogWriter.builder()
.requestLogLevel(parameter.requestLogLevel())
.successfulResponseLogLevel(
parameter.successfulResponseLogLevel())
.failureResponseLogLevel(parameter.failureResponseLogLevel())
.build())
.successSamplingRate(successSamplingRate)
.failureSamplingRate(failureSamplingRate)
.newDecorator();
final LogWriter logWriter = LogWriter.builder()
.requestLogLevel(parameter.requestLogLevel())
.successfulResponseLogLevel(parameter.successfulResponseLogLevel())
.failureResponseLogLevel(parameter.failureResponseLogLevel())
.build();
final LoggingServiceBuilder builder
= LoggingService.builder()
.logWriter(logWriter)
.successSamplingRate(successSamplingRate)
.failureSamplingRate(failureSamplingRate);

if (parameter.slowRequestSamplingPercentile() >= 0.0f) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should users simply use the hard limit slowRequestSamplingUpperBoundMillis without percentile?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think it makes sense 👍

builder.slowRequestSamplingPercentile(parameter.slowRequestSamplingPercentile(),
parameter.slowRequestSamplingWindowMilliseconds(),
parameter.slowRequestSamplingLowerBoundMilliseconds(),
parameter.slowRequestSamplingUpperBoundMilliseconds());
}
return builder.newDecorator();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If isSlow is true, can we skip the additional samplings?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are tests that verify this there is similar behavior here?

void andOrNotShortCircuited() {
final SampleOnce first = new SampleOnce();
final SampleOnce second = new SampleOnce();
assertThat(first.and(second).isSampled(0)).isTrue();
assertThat(first.isSampled(0)).isFalse();
assertThat(second.isSampled(0)).isFalse();
first.reset();
second.reset();
assertThat(first.or(second).isSampled(0)).isTrue();
assertThat(first.isSampled(0)).isFalse();
assertThat(second.isSampled(0)).isFalse();
first.reset();
second.reset();
assertThat(second.and(second).isSampled(0)).isFalse();
}


return successOrFailure || isSlow;
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ public final class LoggingServiceBuilder extends LoggingDecoratorBuilder {

private Sampler<? super ServiceRequestContext> failureSampler = Sampler.always();

private Sampler<Long> slowRequestSampler = Sampler.never();

LoggingServiceBuilder() {}

/**
Expand Down Expand Up @@ -107,6 +109,14 @@ public LoggingServiceBuilder failureSampler(
return this;
}

/**
* Sets the {@link Sampler} that determines whether a request is slow enough to be logged.
*/
public LoggingServiceBuilder slowRequestSampler(Sampler<Long> slowRequestSampler) {
this.slowRequestSampler = requireNonNull(slowRequestSampler, "slowRequestSampler");
return this;
}

/**
* Sets the rate at which to sample requests to log. Any number between {@code 0.0} and {@code 1.0} will
* cause a random sample of the failure requests to be logged.
Expand All @@ -118,12 +128,57 @@ public LoggingServiceBuilder failureSamplingRate(float failureSamplingRate) {
return failureSampler(Sampler.random(failureSamplingRate));
}

/**
* Sets conditions to sample slow requests.
*
* <p>If {slowRequestPercentile} is 0.99, {windowMilliseconds} is 60000, we will sample requests that are
* slower than 99% of the requests within 1-minute time window. This will make sure our sampling adapts the
* traffic pattern. For example, traffic can be very low during the night thus our average response time
* will be pretty low. Therefore, our p99s should be lower than the midday traffic.
* Otherwise, we won't log any p99s during night.</p>
*
* <p>If we set {slowRequestSamplingLowerBoundMilliseconds} to 100, we won't sample any request that took
* less than 1000ms. This is useful to filter out requests that are too fast to be considered
* as slow requests. If your endpoint performs pretty healthy, you shouldn't see any slow request logs.</p>
*
* <p>If we set {slowRequestSamplingUpperBoundMilliseconds} to 1000, we will sample any request that
* took more than 1000 milliseconds. This is useful to make sure we log slow requests even if
* they are not in the p99 percentile. If your service is unhealthy, this will make sure any slow request
* is logged into your system.</p>
*
* @param slowRequestPercentile percentile of slow requests.
* @param windowMilliseconds window size to calculate percentile.
* @param slowRequestSamplingLowerBoundMilliseconds lower bound of slow requests.
* Any request that took less than this amount of time won't be sampled.
* @param slowRequestSamplingUpperBoundMilliseconds upper bound of slow requests.
* Any request that took more than this amount of time will be sampled regardless
* of their percentile.
*/
public LoggingServiceBuilder slowRequestSamplingPercentile(float slowRequestPercentile,
long windowMilliseconds,
long slowRequestSamplingLowerBoundMilliseconds,
long slowRequestSamplingUpperBoundMilliseconds) {
final Sampler<Long> percentileMatches;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we early return if some values are disabled and raise an exception if some of them are illegal?

if ((slowRequestPercentile <= 0.0 || windowMilliseconds <= 0) &&
    slowRequestSamplingLowerBoundMilliseconds == 0 &&
    slowRequestSamplingUpperBoundMilliseconds == Long.MAX_VALUE) {
   return;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I don't think I need to check the lower bound. Lower bound is just for ignoring so I guess users are free to pass there whatever they want regardless.

if (0.0 <= slowRequestPercentile && slowRequestPercentile <= 1.0) {
percentileMatches = Sampler.never();
} else {
percentileMatches = Sampler.percentile(slowRequestPercentile, windowMilliseconds);
}

final Sampler<Long> isSlow = Sampler.greaterThanOrEqual(
slowRequestSamplingLowerBoundMilliseconds * 1000);
final Sampler<Long> isVerySlow = Sampler.greaterThan(slowRequestSamplingUpperBoundMilliseconds * 1000);
return slowRequestSampler(
isVerySlow.or(percentileMatches.and(isSlow))
);
}

/**
* Returns a newly-created {@link LoggingService} decorating {@link HttpService} based on the properties
* of this builder.
*/
public LoggingService build(HttpService delegate) {
return new LoggingService(delegate, logWriter(), successSampler, failureSampler);
return new LoggingService(delegate, logWriter(), successSampler, failureSampler, slowRequestSampler);
}

/**
Expand Down
Loading