Skip to content

Adding Sanitizer interface and implementation #97

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

Open
longquanzheng wants to merge 23 commits into
base:master
Choose a base branch
from
Open

Adding Sanitizer interface and implementation #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0f77624
Adding Sanitizer interface and implementation
ravirajj Sep 7, 2019
afa6c6d
address comments to avoid copy
longquanzheng Apr 8, 2021
a772d57
rename and comment
longquanzheng Apr 8, 2021
a188eee
fix error
longquanzheng Apr 8, 2021
fc8f1aa
rename
longquanzheng Apr 8, 2021
edc0a4e
fix test
longquanzheng Apr 10, 2021
d8fdac9
fix style
longquanzheng Apr 10, 2021
45243d8
adderss comments
longquanzheng Apr 16, 2021
a27e6d3
Use Function interface
longquanzheng Apr 16, 2021
5aa1dc5
fix style
longquanzheng Apr 16, 2021
1b941ff
address comments
longquanzheng Apr 22, 2021
6e4f04a
NoopSanitizer
longquanzheng Apr 22, 2021
5328ba1
adress commeents
longquanzheng Apr 22, 2021
5cb9f0f
address comments for validCharacters
longquanzheng Apr 22, 2021
3e34116
address comments for unit tests
longquanzheng Apr 22, 2021
dde5ff4
use UNDERSCORE_DASH_CHARACTERS
longquanzheng Apr 22, 2021
fbbf111
fix format
longquanzheng Apr 22, 2021
f1d8f25
more tests
longquanzheng Apr 22, 2021
bb561ec
more fix for copyright
longquanzheng Apr 22, 2021
315c559
Bench with sanitizer
longquanzheng Apr 23, 2021
d0c33f8
close at teardown
longquanzheng Apr 23, 2021
08ed2d5
add bench results
longquanzheng Apr 23, 2021
8b09947
fix style
longquanzheng Apr 23, 2021
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
4 changes: 4 additions & 0 deletions core/benchmark-tests.txt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
Benchmark Mode Cnt Score Error Units
ScopeImplBenchmark.recordingBenchmark thrpt 10 74.819 ± 4.573 ops/ms

@alexeykudinkinalexeykudinkinApr 26, 2021

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.

Please check how other benchmarks are reported (we need to capture all data-points offered by JMH)

@longquanzhenglongquanzhengApr 30, 2021•
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, I don't really understand how exactly to do it.

Would you mind taking it over? I am not convinced that this should be done by this PR, as the benchmark is already missing. The most problem is I don't have a clear picture of what you really want for benchmarking(with out detailed documentation), and it should be much easier if your team just do it.

LMK if you agree.

@alexeykudinkinalexeykudinkinMay 1, 2021•
edited
Loading

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.

It should be done by this PR. This is a critical library laying in the hot-path of execution of many services, hence we need to maintain the focus on its performance.

Over the last year a lot of efforts have been put in to optimize, streamline, and make this library robust to serve the needs it was originally built for. Therefore, as a new contribution guideline we require any non-trivial change to adhere to the same basic principles of assuring library's correctness and performance.

Totally appreciate the amount of incremental effort that is required to adhere to this heightened standards from every contribution, but unfortunately there's no other way to guarantee high-level of reliability and performance of the open-source library otherwise.

@longquanzhenglongquanzhengMay 5, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Please check how other benchmarks are reported

Can you give more details here? I did check but don't know how that works(though I see other reports have the details like gc time).
For example another benchmark is just
public class M3ReporterBenchmark extends AbstractReporterBenchmark. So I don't know what am I missing here.

@alexeykudinkinalexeykudinkinMay 7, 2021

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.

@SokolAndrey can you help in here?

ScopeImplBenchmark.recordingWithSanitizingDashBenchmark thrpt 10 49.256 ± 9.474 ops/ms
ScopeImplBenchmark.scopeReportingBenchmark thrpt 10 39066.800 ± 262.844 ops/ms
71 changes: 48 additions & 23 deletions core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -20,23 +20,18 @@

package com.uber.m3.tally;

import com.uber.m3.tally.sanitizers.ScopeSanitizerBuilder;
import com.uber.m3.tally.sanitizers.ValidCharacters;
import com.uber.m3.util.Duration;
import com.uber.m3.util.ImmutableMap;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.TearDown;
import org.openjdk.jmh.annotations.*;

import java.util.Random;
import java.util.concurrent.TimeUnit;

@BenchmarkMode(Mode.Throughput)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@Fork(value = 2, jvmArgsAppend = {"-server", "-XX:+UseG1GC"})
@Fork(value = 2, jvmArgsAppend = {"-server", "-XX:+UseG1GC"})
public class ScopeImplBenchmark {

private static final DurationBuckets EXPONENTIAL_BUCKETS = DurationBuckets.linear(Duration.ofMillis(1), Duration.ofMillis(10), 128);
Expand DownExpand Up@@ -67,28 +62,56 @@ public class ScopeImplBenchmark {

@Benchmark
public void scopeReportingBenchmark(BenchmarkState state) {
state.scope.reportLoopIteration();
state.reportingBenchmarkScope.reportLoopIteration();
}

@Benchmark
public void recordingWithSanitizingDashBenchmark(BenchmarkState state) {
state.recordTestMetrics(state.sanitizingBenchmarkScope);
}

@Benchmark
public void recordingBenchmark(BenchmarkState state) {
state.recordTestMetrics(state.recordingBenchmarkScope);
}

@State(org.openjdk.jmh.annotations.Scope.Benchmark)
public static class BenchmarkState {

private ScopeImpl scope;
private ScopeImpl reportingBenchmarkScope;
private ScopeImpl sanitizingBenchmarkScope;
private ScopeImpl recordingBenchmarkScope;

@Setup
public void setup() {
this.scope =
(ScopeImpl) new RootScopeBuilder()
.reporter(new TestStatsReporter())
.tags(
ImmutableMap.of(
"service", "some-service",
"application", "some-application",
"instance", "some-instance"
)
)
.reportEvery(Duration.MAX_VALUE);
final ScopeBuilder scopeBuilder = new RootScopeBuilder()
.reporter(new TestStatsReporter())
.tags(
ImmutableMap.of(
"service", "some-service",
"application", "some-application",
"instance", "some-instance"
)
);

this.reportingBenchmarkScope =
(ScopeImpl) scopeBuilder.reportEvery(Duration.MAX_VALUE);

this.recordTestMetrics(this.reportingBenchmarkScope);

this.recordingBenchmarkScope = (ScopeImpl) scopeBuilder.reportEvery(Duration.MAX_VALUE);
this.sanitizingBenchmarkScope =
(ScopeImpl) scopeBuilder.sanitizer(
new ScopeSanitizerBuilder()
.withNameValidCharacters(ValidCharacters.of(null, ValidCharacters.UNDERSCORE_CHARACTERS))
.withTagKeyValidCharacters(ValidCharacters.of(null, ValidCharacters.UNDERSCORE_CHARACTERS))
.withTagValueValidCharacters(ValidCharacters.of(null, ValidCharacters.UNDERSCORE_CHARACTERS))
.build()
)
.reportEvery(Duration.MAX_VALUE);
}

public void recordTestMetrics(final ScopeImpl scope) {

@alexeykudinkinalexeykudinkinApr 26, 2021

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 don't think this is a good routine to check -- this was used as pre-init seq, and i would suggest to keep it as such.

Instead create separate benchmarks and routines to update counter/gauge/histogram separately.

@longquanzhenglongquanzhengApr 30, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same as my pervious response. I appreciate your time reviewing and commenting it.
But it would be nice if you can help the benchmarking.

@alexeykudinkinalexeykudinkinMay 1, 2021

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.

Please check my comment above. We're more than happy to help w/ guidance, but benchmarking is now a standard requirement from non-trivial contributions.

@longquanzhenglongquanzhengMay 5, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, will revert that one back.

Can you give me details about what you want to benchmark? -- which method/function/routines?

@alexeykudinkinalexeykudinkinMay 7, 2021•
edited
Loading

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.

As i have called out prior, let's test creation and recording fro each type of metric individually (counter/gauge/histogram).

// Counter
scope.counter("counter").inc(1)
// Timer
scope.timer("timer").record(...)
// Histogram
...

for (String counterName : COUNTER_NAMES) {
scope.counter(counterName).inc(1);
}
Expand All@@ -112,7 +135,9 @@ public void setup() {

@TearDown
public void teardown() {
scope.close();
reportingBenchmarkScope.close();
recordingBenchmarkScope.close();
sanitizingBenchmarkScope.close();
}

}
Expand Down
14 changes: 14 additions & 0 deletions core/src/main/java/com/uber/m3/tally/ScopeBuilder.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@

package com.uber.m3.tally;

import com.uber.m3.tally.sanitizers.NoopSanitizer;
import com.uber.m3.tally.sanitizers.ScopeSanitizer;
import com.uber.m3.util.Duration;
import com.uber.m3.util.ImmutableMap;

Expand DownExpand Up@@ -55,6 +57,7 @@ public class ScopeBuilder {
protected String separator = DEFAULT_SEPARATOR;
protected ImmutableMap<String, String> tags;
protected Buckets defaultBuckets = DEFAULT_SCOPE_BUCKETS;
protected ScopeSanitizer sanitizer = new NoopSanitizer();

private ScheduledExecutorService scheduler;
private ScopeImpl.Registry registry;
Expand DownExpand Up@@ -128,6 +131,17 @@ public ScopeBuilder defaultBuckets(Buckets defaultBuckets) {
return this;
}

/**
* Update the sanitizer.
*
* @param sanitizer value to update to
* @return Builder with new param updated
*/
public ScopeBuilder sanitizer(ScopeSanitizer sanitizer) {
this.sanitizer = sanitizer;
return this;
}

// Private build method - clients should rely on `reportEvery` to create root scopes, and
// a root scope's `tagged` and `subScope` functions to create subscopes.
ScopeImpl build() {
Expand Down
113 changes: 73 additions & 40 deletions core/src/main/java/com/uber/m3/tally/ScopeImpl.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,7 @@

package com.uber.m3.tally;

import com.uber.m3.tally.sanitizers.ScopeSanitizer;
import com.uber.m3.util.ImmutableMap;

import javax.annotation.Nullable;
Expand All@@ -41,6 +42,7 @@ class ScopeImpl implements Scope {
private String separator;
private ImmutableMap<String, String> tags;
private Buckets defaultBuckets;
private ScopeSanitizer sanitizer;

private ScheduledExecutorService scheduler;
private Registry registry;
Expand All@@ -57,6 +59,7 @@ class ScopeImpl implements Scope {
ScopeImpl(ScheduledExecutorService scheduler, Registry registry, ScopeBuilder builder) {
this.scheduler = scheduler;
this.registry = registry;
this.sanitizer = builder.sanitizer;

this.reporter = builder.reporter;
this.prefix = builder.prefix;
Expand All@@ -67,33 +70,37 @@ class ScopeImpl implements Scope {

@Override
public Counter counter(String name) {
return counters.computeIfAbsent(name, ignored ->
final String finalName = sanitizer.sanitizeName(name);
return counters.computeIfAbsent(finalName, ignored ->
// NOTE: This will called at most once
new CounterImpl(this, fullyQualifiedName(name))
new CounterImpl(this, fullyQualifiedName(finalName))
);
}

@Override
public Gauge gauge(String name) {
return gauges.computeIfAbsent(name, ignored ->
final String finalName = sanitizer.sanitizeName(name);
return gauges.computeIfAbsent(finalName, ignored ->
// NOTE: This will called at most once
new GaugeImpl(this, fullyQualifiedName(name)));
new GaugeImpl(this, fullyQualifiedName(finalName)));
}

@Override
public Timer timer(String name) {
final String finalName = sanitizer.sanitizeName(name);
// Timers report directly to the {@code StatsReporter}, and therefore not added to reporting queue
// i.e. they are not buffered
return timers.computeIfAbsent(name, ignored -> new TimerImpl(fullyQualifiedName(name), tags, reporter));
return timers.computeIfAbsent(finalName, ignored -> new TimerImpl(fullyQualifiedName(finalName), tags, reporter));
}

@Override
public Histogram histogram(String name, @Nullable Buckets buckets) {
return histograms.computeIfAbsent(name, ignored ->
final String finalName = sanitizer.sanitizeName(name);
return histograms.computeIfAbsent(finalName, ignored ->
// NOTE: This will called at most once
new HistogramImpl(
this,
fullyQualifiedName(name),
fullyQualifiedName(finalName),
tags,
Optional.ofNullable(buckets)
.orElse(defaultBuckets)
Expand All@@ -103,12 +110,13 @@ public Histogram histogram(String name, @Nullable Buckets buckets) {

@Override
public Scope tagged(Map<String, String> tags) {
return subScopeHelper(prefix, tags);
return subScopeHelper(prefix, sanitizeTags(tags));
}

@Override
public Scope subScope(String name) {
return subScopeHelper(fullyQualifiedName(name), null);
final String finalName = sanitizer.sanitizeName(name);
return subScopeHelper(fullyQualifiedName(finalName), null);
}

@Override
Expand DownExpand Up@@ -136,12 +144,35 @@ public void close() {
}
}

private Map<String, String> sanitizeTags(Map<String, String> tags) {
if (tags == null) {
return null;
}

boolean hasChange = false;
for (Map.Entry<String, String> kv : tags.entrySet()) {
if (!sanitizer.sanitizeTagKey(kv.getKey()).equals(kv.getKey()) || !sanitizer.sanitizeTagValue(kv.getValue()).equals(kv.getValue())) {
hasChange = true;
break;
}
}
if (!hasChange) {
return tags;
}

ImmutableMap.Builder<String, String> builder = new ImmutableMap.Builder<>();
tags.forEach((key, value) -> builder.put(sanitizer.sanitizeTagKey(key), sanitizer.sanitizeTagValue(value)));

return builder.build();
}

<T extends Reportable> void addToReportingQueue(T metric) {
reportingQueue.add(metric);
}

/**
* Reports using the specified reporter.
*
* @param reporter the reporter to report
*/
void report(StatsReporter reporter) {
Expand DownExpand Up@@ -193,6 +224,7 @@ String fullyQualifiedName(String name) {

/**
* Returns a {@link Snapshot} of this {@link Scope}.
*
* @return a {@link Snapshot} of this {@link Scope}
*/
public Snapshot snapshot() {
Expand All@@ -205,12 +237,12 @@ public Snapshot snapshot() {
String id = keyForPrefixedStringMap(name, tags);

snap.counters().put(
id,
new CounterSnapshotImpl(
name,
tags,
counter.getValue().snapshot()
)
id,
new CounterSnapshotImpl(
name,
tags,
counter.getValue().snapshot()
)
);
}

Expand All@@ -220,12 +252,12 @@ public Snapshot snapshot() {
String id = keyForPrefixedStringMap(name, tags);

snap.gauges().put(
id,
new GaugeSnapshotImpl(
name,
tags,
gauge.getValue().snapshot()
)
id,
new GaugeSnapshotImpl(
name,
tags,
gauge.getValue().snapshot()
)
);
}

Expand All@@ -235,12 +267,12 @@ public Snapshot snapshot() {
String id = keyForPrefixedStringMap(name, tags);

snap.timers().put(
id,
new TimerSnapshotImpl(
name,
tags,
timer.getValue().snapshot()
)
id,
new TimerSnapshotImpl(
name,
tags,
timer.getValue().snapshot()
)
);
}

Expand All@@ -250,13 +282,13 @@ public Snapshot snapshot() {
String id = keyForPrefixedStringMap(name, tags);

snap.histograms().put(
id,
new HistogramSnapshotImpl(
name,
tags,
histogram.getValue().snapshotValues(),
histogram.getValue().snapshotDurations()
)
id,
new HistogramSnapshotImpl(
name,
tags,
histogram.getValue().snapshotValues(),
histogram.getValue().snapshotDurations()
)
);
}
}
Expand All@@ -283,12 +315,13 @@ private Scope subScopeHelper(String prefix, Map<String, String> tags) {
return registry.subscopes.computeIfAbsent(
key,
(k) -> new ScopeBuilder(scheduler, registry)
.reporter(reporter)
.prefix(prefix)
.separator(separator)
.tags(mergedTags)
.defaultBuckets(defaultBuckets)
.build()
.reporter(reporter)
.prefix(prefix)
.separator(separator)
.tags(mergedTags)
.sanitizer(sanitizer)
.defaultBuckets(defaultBuckets)
.build()
);
}

Expand Down
Loading