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.

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

Adds resource name matching to the trace sampling rules #5900

Merged
merged 7 commits into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public static RuleBasedTraceSampler build(
new TraceSamplingRule(
rule.getService(),
rule.getName(),
rule.getResource(),
new DeterministicSampler.TraceSampler(rule.getSampleRate()));
samplingRules.add(samplingRule);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package datadog.trace.common.sampling;

import datadog.trace.core.CoreSpan;
import datadog.trace.core.util.GlobPattern;
import datadog.trace.core.util.Matcher;
import datadog.trace.core.util.Matchers;
import datadog.trace.core.util.SimpleRateLimiter;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -74,28 +75,32 @@ protected <T extends CoreSpan<T>> CharSequence getRelevantString(final T span) {
}

public static final class TraceSamplingRule extends SamplingRule {
private final String serviceName;
private final String operationName;
private final Matcher serviceMatcher;
private final Matcher operationMatcher;
private final Matcher resourceMatcher;

public TraceSamplingRule(
final String exactServiceName, final String exactOperationName, final RateSampler sampler) {
final String serviceGlob,
final String operationGlob,
final String resourceGlob,
final RateSampler sampler) {
super(sampler);
this.serviceName = exactServiceName;
this.operationName = exactOperationName;
serviceMatcher = Matchers.compileGlob(serviceGlob);
operationMatcher = Matchers.compileGlob(operationGlob);
resourceMatcher = Matchers.compileGlob(resourceGlob);
}

@Override
public <T extends CoreSpan<T>> boolean matches(T span) {
return (serviceName == null || serviceName.equals(span.getServiceName()))
&& (operationName == null || operationName.contentEquals(span.getOperationName()));
return Matchers.matches(serviceMatcher, span.getServiceName())
&& Matchers.matches(operationMatcher, span.getOperationName())
&& Matchers.matches(resourceMatcher, span.getResourceName());
}
}

public static final class SpanSamplingRule extends SamplingRule {
private final String serviceExactName;
private final Pattern servicePattern;
private final String operationExactName;
private final Pattern operationPattern;
private final Matcher serviceMatcher;
private final Matcher operationMatcher;

private final SimpleRateLimiter rateLimiter;

Expand All @@ -106,43 +111,16 @@ public SpanSamplingRule(
final SimpleRateLimiter rateLimiter) {
super(sampler);

if (serviceName == null || "*".equals(serviceName)) {
this.serviceExactName = null;
this.servicePattern = null;
} else if (isExactMatcher(serviceName)) {
this.serviceExactName = serviceName;
this.servicePattern = null;
} else {
this.serviceExactName = null;
this.servicePattern = GlobPattern.globToRegexPattern(serviceName);
}

if (operationName == null || "*".equals(operationName)) {
this.operationExactName = null;
this.operationPattern = null;
} else if (isExactMatcher(operationName)) {
this.operationExactName = operationName;
this.operationPattern = null;
} else {
this.operationExactName = null;
this.operationPattern = GlobPattern.globToRegexPattern(operationName);
}
serviceMatcher = Matchers.compileGlob(serviceName);
operationMatcher = Matchers.compileGlob(operationName);

this.rateLimiter = rateLimiter;
}

private boolean isExactMatcher(String serviceNameGlob) {
return !serviceNameGlob.contains("*") && !serviceNameGlob.contains("?");
}

@Override
public <T extends CoreSpan<T>> boolean matches(T span) {
return (serviceExactName == null || serviceExactName.equals(span.getServiceName()))
&& (servicePattern == null || servicePattern.matcher(span.getServiceName()).matches())
&& (operationExactName == null
|| operationExactName.contentEquals(span.getOperationName()))
&& (operationPattern == null
|| operationPattern.matcher(span.getOperationName()).matches());
return Matchers.matches(serviceMatcher, span.getServiceName())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice clean up!

&& Matchers.matches(operationMatcher, span.getOperationName());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,49 +17,62 @@ public class TraceSamplingRules {
private static final Logger log = LoggerFactory.getLogger(TraceSamplingRules.class);

public static final class Rule {
public static Rule create(String service, String name, double sampleRate) {
public static Rule create(String service, String name, String resource, double sampleRate) {
if (sampleRate < 0 || sampleRate > 1.0) {
logError(
service, name, Double.toString(sampleRate), "sample_rate must be between 0.0 and 1.0");
service,
name,
resource,
Double.toString(sampleRate),
"sample_rate must be between 0.0 and 1.0");
return null;
}
return new Rule(service, name, sampleRate);
return new Rule(service, name, resource, sampleRate);
}

public static Rule create(String service, String name, String sample_rate) {
public static Rule create(String service, String name, String resource, String sample_rate) {
if (sample_rate == null) {
logError(service, name, null, "missing mandatory sample_rate");
logError(service, name, resource, null, "missing mandatory sample_rate");
return null;
}
try {
return create(service, name, Double.parseDouble(sample_rate));
return create(service, name, resource, Double.parseDouble(sample_rate));
} catch (NumberFormatException ex) {
logError(service, name, sample_rate, "sample_rate must be a number between 0.0 and 1.0");
logError(
service,
name,
resource,
sample_rate,
"sample_rate must be a number between 0.0 and 1.0");
return null;
}
}

private static void logError(String service, String name, String sample_rate, String error) {
private static void logError(
String service, String name, String resource, String sample_rate, String error) {
log.error(
"Skipping invalid Trace Sampling Rule: { \"service\": \""
+ service
+ "\", \"name\": \""
+ name
+ "\", \"resource\": \""
+ resource
+ "\", \"sample_rate\": "
+ sample_rate
+ " } - "
+ error);
}

private final String service;

private final String name;
private final String resource;

private final double sampleRate;

private Rule(String service, String name, double sampleRate) {
private Rule(String service, String name, String resource, double sampleRate) {
this.service = service;
this.name = name;
this.resource = resource;
this.sampleRate = sampleRate;
}

Expand All @@ -71,6 +84,10 @@ public String getName() {
return name;
}

public String getResource() {
return resource;
}

public double getSampleRate() {
return sampleRate;
}
Expand All @@ -88,10 +105,11 @@ public static TraceSamplingRules deserialize(String json) {
private static final class JsonRule {
String service;
String name;
String resource;
String sample_rate;

private Rule toRule() {
return Rule.create(service, name, sample_rate);
return Rule.create(service, name, resource, sample_rate);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,11 @@
public final class GlobPattern {

public static Pattern globToRegexPattern(String globPattern) {
if (globPattern == null) {
Copy link
Contributor

@ygree ygree Nov 18, 2023

Choose a reason for hiding this comment

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

Removed now unnecessary checks in these methods

return null;
}
String regex = globToRegex(globPattern);
if (regex == null) {
return null;
}
return Pattern.compile(regex);
}

private static String globToRegex(String globPattern) {
if ("*".equals(globPattern)) {
return null;
}
StringBuilder sb = new StringBuilder(64);
sb.append('^');
for (int i = 0; i < globPattern.length(); i++) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package datadog.trace.core.util;

public interface Matcher {
public boolean matches(String str);

public boolean matches(CharSequence charSeq);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package datadog.trace.core.util;

import java.util.regex.Pattern;

public final class Matchers {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some unit tests here to codify the edge cases?

I don't have many concerns about the correctness of the code, but it would be nice to have our edge cases explicitly defined

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are ready for another review

private Matchers() {}

public static final Matcher compileGlob(String glob) {
if (glob == null || glob.equals("*")) {
bm1549 marked this conversation as resolved.
Show resolved Hide resolved
// DQH - Decided not to an anyMatcher because that's likely to
// cause our call site to go megamorphic
return null;
} else if (isExact(glob)) {
return new ExactMatcher(glob);
} else {
// DQH - not sure about the error handling here
Pattern pattern = GlobPattern.globToRegexPattern(glob);
return pattern == null ? null : new PatternMatcher(pattern);
}
}

public static boolean matches(Matcher matcher, String str) {
return (matcher == null) || matcher.matches(str);
}

public static boolean matches(Matcher matcher, CharSequence charSeq) {
return (matcher == null) || matcher.matches(charSeq);
}

static boolean isExact(String glob) {
return (glob.indexOf('*') == -1) && (glob.indexOf('?') == -1);
}

static final class ExactMatcher implements Matcher {
private final String exact;

ExactMatcher(String exact) {
this.exact = exact;
}

@Override
public boolean matches(String str) {
return exact.equals(str);
}

@Override
public boolean matches(CharSequence charSeq) {
return exact.contentEquals(charSeq);
}
}

static final class PatternMatcher implements Matcher {
private final Pattern pattern;

PatternMatcher(Pattern pattern) {
this.pattern = pattern;
}

@Override
public boolean matches(CharSequence charSeq) {
return pattern.matcher(charSeq).matches();
}

@Override
public boolean matches(String str) {
return pattern.matcher(str).matches();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,9 @@ class RuleBasedSamplingTest extends DDCoreSpecification {
DDSpan span = tracer.buildSpan("operation")
.withServiceName("service")
.withTag("env", "bar")
.ignoreActiveSpan().start()
.withResourceName("resource")
.ignoreActiveSpan()
.start()
((PrioritySampler) sampler).setSamplingPriority(span)

then:
Expand Down Expand Up @@ -214,6 +216,20 @@ class RuleBasedSamplingTest extends DDCoreSpecification {
"[{\"name\": \"xxx\", \"sample_rate\": 0}, {\"name\": \"operation\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"name\": \"xxx\", \"sample_rate\": 1}, {\"name\": \"operation\", \"sample_rate\": 0}]" | null | 0 | null | null | USER_DROP

// Matching resource : keep
"[{\"resource\": \"resource\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP

// Matching operation: drop
"[{\"resource\": \"resource\", \"sample_rate\": 0}]" | null | 0 | null | null | USER_DROP

// Matching operation overrides default rate
"[{\"resource\": \"resource\", \"sample_rate\": 1}]" | "0" | 1.0 | 50 | null | USER_KEEP
"[{\"resource\": \"resource\", \"sample_rate\": 0}]" | "1" | 0 | null | null | USER_DROP

// multiple operation combinations
"[{\"resource\": \"xxx\", \"sample_rate\": 0}, {\"resource\": \"resource\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"resource\": \"xxx\", \"sample_rate\": 1}, {\"resource\": \"resource\", \"sample_rate\": 0}]" | null | 0 | null | null | USER_DROP

// Service and operation name rules
"[{\"service\": \"service\", \"sample_rate\": 1}, {\"name\": \"operation\", \"sample_rate\": 0}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"service\": \"service\", \"sample_rate\": 1}, {\"name\": \"xxx\", \"sample_rate\": 0}]" | null | 1.0 | 50 | null | USER_KEEP
Expand All @@ -227,6 +243,7 @@ class RuleBasedSamplingTest extends DDCoreSpecification {
"[{\"service\": \"service\", \"name\": \"xxx\", \"sample_rate\": 0}, {\"service\": \"service\", \"name\": \"operation\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"service\": \"service\", \"name\": \"xxx\", \"sample_rate\": 0}, {\"service\": \"service\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"service\": \"service\", \"name\": \"xxx\", \"sample_rate\": 0}, {\"name\": \"operation\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"service\": \"service\", \"resource\": \"xxx\", \"sample_rate\": 0}, {\"resource\": \"resource\", \"sample_rate\": 1}]" | null | 1.0 | 50 | null | USER_KEEP
"[{\"service\": \"service\", \"name\": \"operation\", \"sample_rate\": 0}, {\"service\": \"service\", \"name\": \"operation\", \"sample_rate\": 1}]" | null | 0 | null | null | USER_DROP
"[{\"service\": \"service\", \"name\": \"operation\", \"sample_rate\": 0}]" | null | 0 | null | null | USER_DROP
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,37 @@ class TraceSamplingRulesTest extends DDCoreSpecification {
{\"service\": \"usersvc\", \"name\": \"healthcheck\", \"sample_rate\": 0.0},
{\"service\": \"service-name\", \"sample_rate\": 0.5},
{\"name\": \"operation-name\", \"sample_rate\": 0.75},
{\"resource\": \"resource-name\", \"sample_rate\": 1},
{\"sample_rate\": 0.25}
]""")

then:
rules.rules.size() == 4
rules.rules.size() == 5

rules.rules[0].service == "usersvc"
rules.rules[0].name == "healthcheck"
rules.rules[0].resource == null
rules.rules[0].sampleRate == 0.0d

rules.rules[1].service == "service-name"
rules.rules[1].name == null
rules.rules[1].resource == null
rules.rules[1].sampleRate == 0.5d

rules.rules[2].service == null
rules.rules[2].name == "operation-name"
rules.rules[2].resource == null
rules.rules[2].sampleRate == 0.75d

rules.rules[3].service == null
rules.rules[3].name == null
rules.rules[3].sampleRate == 0.25d
rules.rules[3].resource == "resource-name"
rules.rules[3].sampleRate == 1d

rules.rules[4].service == null
rules.rules[4].name == null
rules.rules[4].resource == null
rules.rules[4].sampleRate == 0.25d
}

def "Skip Trace Sampling Rules with invalid rate values"() {
Expand Down
Loading
Loading