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

[DGS-18780] Ensure p99 metrics are correctly published #519

Merged
merged 4 commits into from
Jan 15, 2025
+32 −13
Merged

[DGS-18780] Ensure p99 metrics are correctly published #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94b6b72
Make percentile max latency configurable
pritishatx Dec 13, 2024
61a6e07
Merge branch 'master' of https://github.com/confluentinc/rest-utils i…
pritishatx Dec 13, 2024
5066cb4
minor changes
pritishatx Dec 14, 2024
016fc09
checkstyle changes
pritishatx Dec 17, 2024
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
1 change: 1 addition & 0 deletions core/src/main/java/io/confluent/rest/Application.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,7 @@ public void configureBaseApplication(Configurable<?> config, Map<String, String>
restConfig.getBoolean(RestConfig.METRICS_LATENCY_SLO_SLA_ENABLE_CONFIG),
restConfig.getLong(RestConfig.METRICS_LATENCY_SLO_MS_CONFIG),
restConfig.getLong(RestConfig.METRICS_LATENCY_SLA_MS_CONFIG),
restConfig.getDouble(RestConfig.PERCENTILE_MAX_LATENCY_MS_CONFIG),
restConfig.getBoolean(RestConfig.METRICS_GLOBAL_STATS_REQUEST_TAGS_ENABLE_CONFIG)));

config.property(ServerProperties.BV_SEND_ERROR_IN_RESPONSE, true);
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/io/confluent/rest/RestConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ public class RestConfig extends AbstractConfig {
protected static final String METRICS_LATENCY_SLA_MS_DOC = "The threshold (in ms) of whether"
+ " request latency meets or violates SLA";
protected static final long METRICS_LATENCY_SLA_MS_DEFAULT = 50;
public static final String PERCENTILE_MAX_LATENCY_MS_CONFIG = "percentile.max.latency.ms";
protected static final String PERCENTILE_MAX_LATENCY_MS_DOC = "The threshold (in ms) of"
+ " percentile maximum latency";
protected static final double PERCENTILE_MAX_LATENCY_MS_DEFAULT = 10000;
public static final String METRICS_GLOBAL_STATS_REQUEST_TAGS_ENABLE_CONFIG =
"metrics.global.stats.request.tags.enable";
protected static final String METRICS_GLOBAL_STATS_REQUEST_TAGS_ENABLE_DOC = "Whether to use "
Expand Down Expand Up @@ -761,6 +765,12 @@ private static ConfigDef incompleteBaseConfigDef() {
METRICS_LATENCY_SLA_MS_DEFAULT,
Importance.LOW,
METRICS_LATENCY_SLA_MS_DOC
).define(
PERCENTILE_MAX_LATENCY_MS_CONFIG,
Type.DOUBLE,
PERCENTILE_MAX_LATENCY_MS_DEFAULT,
Copy link
Member

Choose a reason for hiding this comment

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

[minor] we should have put the validation here to avoid non positive value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me get back with a fix for this.

Copy link
Member

Choose a reason for hiding this comment

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

thanks @pritishatx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @trnguyencflt, raised a PR to add the validation.

Importance.LOW,
PERCENTILE_MAX_LATENCY_MS_DOC
).define(
METRICS_GLOBAL_STATS_REQUEST_TAGS_ENABLE_CONFIG,
Type.BOOLEAN,
Expand Down
34 changes: 21 additions & 13 deletions core/src/main/java/io/confluent/rest/metrics/MetricsResourceMethodApplicationListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ public class MetricsResourceMethodApplicationListener implements ApplicationEven
protected static final String[] HTTP_STATUS_CODE_TEXT = {
"unknown", "1xx", "2xx", "3xx", "4xx", "5xx", "429"};
private static final int PERCENTILE_NUM_BUCKETS = 200;
private static final double PERCENTILE_MAX_LATENCY_IN_MS = TimeUnit.SECONDS.toMillis(10);
private static final long SENSOR_EXPIRY_SECONDS = TimeUnit.HOURS.toSeconds(1);

private final Metrics metrics;
Expand All @@ -81,23 +80,28 @@ public class MetricsResourceMethodApplicationListener implements ApplicationEven
private final boolean enableLatencySloSla;
private final long latencySloMs;
private final long latencySlaMs;
private final double percentileMaxLatencyInMs;

// This controls whether we should use request tags in global stats, i.e., those without resource
// method in the names, introducing this variable to keep the compatibility with downstream
// dependencies, e.g., some applications may not want to report request tags in global stats
private final boolean enableGlobalStatsRequestTags;

public MetricsResourceMethodApplicationListener(Metrics metrics, String metricGrpPrefix,
Map<String, String> metricTags, Time time,
boolean enableLatencySloSla,
long latencySloMs, long latencySlaMs) {
Map<String, String> metricTags, Time time,
boolean enableLatencySloSla,
long latencySloMs, long latencySlaMs,
double percentileMaxLatencyInMs) {
this(metrics, metricGrpPrefix, metricTags, time, enableLatencySloSla, latencySloMs,
latencySlaMs, false);
latencySlaMs, percentileMaxLatencyInMs, false);
}

public MetricsResourceMethodApplicationListener(Metrics metrics, String metricGrpPrefix,
Map<String, String> metricTags, Time time,
boolean enableLatencySloSla,
long latencySloMs, long latencySlaMs, boolean enableGlobalStatsRequestTags) {
Map<String, String> metricTags, Time time,
boolean enableLatencySloSla,
long latencySloMs, long latencySlaMs,
double percentileMaxLatencyInMs,
boolean enableGlobalStatsRequestTags) {
super();
this.metrics = metrics;
this.metricGrpPrefix = metricGrpPrefix;
Expand All @@ -106,6 +110,7 @@ public MetricsResourceMethodApplicationListener(Metrics metrics, String metricGr
this.enableLatencySloSla = enableLatencySloSla;
this.latencySloMs = latencySloMs;
this.latencySlaMs = latencySlaMs;
this.percentileMaxLatencyInMs = percentileMaxLatencyInMs;
this.enableGlobalStatsRequestTags = enableGlobalStatsRequestTags;
}

Expand All @@ -115,7 +120,7 @@ public void onEvent(ApplicationEvent event) {
// Special null key is used for global stats
MethodMetrics m = new MethodMetrics(
null, null, this.metrics, metricGrpPrefix, metricTags, emptyMap(),
enableLatencySloSla, latencySloMs, latencySlaMs);
enableLatencySloSla, latencySloMs, latencySlaMs, percentileMaxLatencyInMs);
methodMetrics.put(null, new RequestScopedMetrics(m, new ConstructionContext(this)));

for (final Resource resource : event.getResourceModel().getResources()) {
Expand All @@ -139,7 +144,7 @@ private void register(ResourceMethod method) {

MethodMetrics m = new MethodMetrics(
method, annotation, metrics, metricGrpPrefix, metricTags, emptyMap(),
enableLatencySloSla, latencySloMs, latencySlaMs);
enableLatencySloSla, latencySloMs, latencySlaMs, percentileMaxLatencyInMs);
ConstructionContext context = new ConstructionContext(method, annotation, this);
methodMetrics.put(definitionMethod, new RequestScopedMetrics(m, context));
}
Expand Down Expand Up @@ -235,17 +240,19 @@ private static class MethodMetrics {
private final boolean enableLatencySloSla;
private final long latencySloMs;
private final long latencySlaMs;
private final double percentileMaxLatencyInMs;

public MethodMetrics(ResourceMethod method, PerformanceMetric annotation, Metrics metrics,
String metricGrpPrefix, Map<String, String> metricTags,
Map<String, String> requestTags) {
this(method, annotation, metrics, metricGrpPrefix, metricTags, requestTags, false, 0L, 0L);
this(method, annotation, metrics, metricGrpPrefix, metricTags, requestTags, false,
0L, 0L, 10000);
}

public MethodMetrics(ResourceMethod method, PerformanceMetric annotation, Metrics metrics,
String metricGrpPrefix, Map<String, String> metricTags,
Map<String, String> requestTags, boolean enableLatencySloSla,
long latencySloMs, long latencySlaMs) {
long latencySloMs, long latencySlaMs, double percentileMaxLatencyInMs) {
String metricGrpName = metricGrpPrefix + "-metrics";
// The tags will be used to generate MBean names if JmxReporter is used,
// sort to get consistent names
Expand Down Expand Up @@ -315,14 +322,15 @@ public MethodMetrics(ResourceMethod method, PerformanceMetric annotation, Metric
this.enableLatencySloSla = enableLatencySloSla;
this.latencySloMs = latencySloMs;
this.latencySlaMs = latencySlaMs;
this.percentileMaxLatencyInMs = percentileMaxLatencyInMs;
if (enableLatencySloSla) {
setResponseLatencySloSlaSensors(method, annotation, metrics, requestTags,
metricGrpName, allTags);
}

Percentiles percs = new Percentiles(Float.SIZE / 8 * PERCENTILE_NUM_BUCKETS,
0.0,
PERCENTILE_MAX_LATENCY_IN_MS,
percentileMaxLatencyInMs,
Percentiles.BucketSizing.LINEAR,
new Percentile(new MetricName(
getName(method, annotation, "request-latency-95"), metricGrpName,
Expand Down