Skip to content

Remove Guava dependencies from SDK #2148

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

Merged
jkwatson merged 2 commits into from
Dec 1, 2020
Merged

Remove Guava dependencies from SDK #2148

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d962c1c
Remove Guava from SDK
Nov 27, 2020
d8476a2
Clean
Nov 28, 2020
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
3 changes: 1 addition & 2 deletions sdk/all/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ dependencies {
project(':opentelemetry-sdk-metrics'),
project(':opentelemetry-sdk-trace')

implementation libraries.guava

annotationProcessor libraries.auto_value

testAnnotationProcessor libraries.auto_value
Expand All @@ -32,6 +30,7 @@ dependencies {
// dependencies.
transitive = false
}
jmh libraries.guava

signature libraries.android_signature
}
Expand Down
2 changes: 0 additions & 2 deletions sdk/all/src/main/java/io/opentelemetry/sdk/OpenTelemetrySdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import static java.util.Objects.requireNonNull;

import com.google.common.annotations.VisibleForTesting;
import io.opentelemetry.api.DefaultOpenTelemetry;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.internal.Obfuscated;
Expand Down Expand Up @@ -253,7 +252,6 @@ private MeterProvider buildMeterProvider() {
* @see Obfuscated
*/
@ThreadSafe
@VisibleForTesting
Comment thread
jkwatson marked this conversation as resolved.
static class ObfuscatedTracerProvider implements TracerProvider, Obfuscated<TracerProvider> {

private final TracerProvider delegate;
Expand Down
2 changes: 0 additions & 2 deletions sdk/common/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ ext.propertiesDir = "build/generated/properties/io/opentelemetry/sdk/common"
dependencies {
api project(':opentelemetry-api')

implementation libraries.guava

annotationProcessor libraries.auto_value

testAnnotationProcessor libraries.auto_value
Expand Down
8 changes: 4 additions & 4 deletions sdk/common/src/main/java/io/opentelemetry/sdk/common/export/ConfigBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.sdk.common.export;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Maps;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand All @@ -26,7 +24,7 @@
*/
public abstract class ConfigBuilder<T> {

@VisibleForTesting
// Visible for testing
protected enum NamingConvention {
DOT {
@Override
Expand Down Expand Up @@ -65,7 +63,9 @@ protected abstract T fromConfigMap(

/** Sets the configuration values from the given {@link Properties} object. */
public T readProperties(Properties properties) {
return fromConfigMap(Maps.fromProperties(properties), NamingConvention.DOT);
Map<String, String> map = new HashMap<>(properties.size());
properties.forEach((key, value) -> map.put((String) key, (String) value));

@Oberon00 Oberon00 Nov 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a normal foreach loop would be more readable.

@anuraaga anuraaga Nov 27, 2020

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've always appreciated Java 8's Map.forEach since Entry can be a bit gross :)

@Oberon00 Oberon00 Nov 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alternatively, how about some casting? https://stackoverflow.com/a/17209434/2128694

Map<String, String> map = (Map<String, String>)(Map)properties;

@anuraaga anuraaga Nov 28, 2020

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.

Thanks!

return fromConfigMap(map, NamingConvention.DOT);
}

/** Sets the configuration values from environment variables. */
Expand Down
3 changes: 1 addition & 2 deletions sdk/common/src/main/java/io/opentelemetry/sdk/resources/EnvAutodetectResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.sdk.resources;

import com.google.common.annotations.VisibleForTesting;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.sdk.common.export.ConfigBuilder;
Expand Down Expand Up @@ -33,7 +32,7 @@ private EnvAutodetectResource() {}
* Values may be quoted or unquoted in general.
* If a value contains whitespaces, =, or " characters, it must always be quoted.
*/
@VisibleForTesting
// Visible for testing
static Attributes parseResourceAttributes(@Nullable String rawEnvAttributes) {
if (rawEnvAttributes == null) {
return Attributes.empty();
Expand Down
24 changes: 14 additions & 10 deletions sdk/common/src/main/java/io/opentelemetry/sdk/resources/ResourcesConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@

package io.opentelemetry.sdk.resources;

import static java.util.Objects.requireNonNull;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableSet;
import io.opentelemetry.sdk.common.export.ConfigBuilder;
import java.util.Arrays;
import java.util.Collections;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.concurrent.Immutable;

/**
Expand Down Expand Up @@ -69,7 +70,8 @@ public static ResourcesConfig getDefault() {
* @return a new {@link Builder}.
*/
public static Builder builder() {
return new AutoValue_ResourcesConfig.Builder().setDisabledResourceProviders(ImmutableSet.of());
return new AutoValue_ResourcesConfig.Builder()
.setDisabledResourceProviders(Collections.emptySet());
}

/**
Expand All @@ -94,7 +96,7 @@ public abstract static class Builder extends ConfigBuilder<Builder> {
* @param configMap {@link Map} holding the configuration values.
* @return this
*/
@VisibleForTesting
// Visible for testing
@Override
protected Builder fromConfigMap(
Map<String, String> configMap, NamingConvention namingConvention) {
Expand All @@ -103,8 +105,11 @@ protected Builder fromConfigMap(
String stringValue = getStringProperty(OTEL_JAVA_DISABLED_RESOURCES_PROVIDERS, configMap);
if (stringValue != null) {
this.setDisabledResourceProviders(
ImmutableSet.copyOf(
Splitter.on(',').omitEmptyStrings().trimResults().split(stringValue)));
Collections.unmodifiableSet(
Arrays.stream(stringValue.split(","))
.map(String::trim)

@Oberon00 Oberon00 Nov 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you might need to swap the order of trim/isEmpty to match Guava. AFAIK omitEmptyStrings only sets a boolean. But I may be wrong here.

@jkwatson jkwatson Nov 30, 2020

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.

Whether or not it matches guava, this logic seems right. Values that are only whitespace shouldn't be used, I don't think.

@anuraaga anuraaga Dec 1, 2020

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.

My not splitting the PRs up in advance caused this review to be a bit hectic, sorry about that. While GitHub doesn't have the outdated mark here, if I remember correctly, the code used to be in the wrong order and @Oberon00 commented and I fixed it so it should be good now.

.filter(s -> !s.isEmpty())
.collect(Collectors.toSet())));
}
return this;
}
Expand All @@ -128,8 +133,7 @@ protected Builder fromConfigMap(
*/
public ResourcesConfig build() {
ResourcesConfig resourcesConfig = autoBuild();
Preconditions.checkArgument(
resourcesConfig.getDisabledResourceProviders() != null, "disabledResourceProviders");
requireNonNull(resourcesConfig.getDisabledResourceProviders(), "disabledResourceProviders");
return resourcesConfig;
}
}
Expand Down
3 changes: 1 addition & 2 deletions sdk/metrics/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,14 @@ dependencies {
api project(':opentelemetry-api'),
project(':opentelemetry-sdk-common')

implementation libraries.guava

annotationProcessor libraries.auto_value

testAnnotationProcessor libraries.auto_value
testCompileOnly libraries.auto_value_annotation

testCompile project(path: ':opentelemetry-sdk-common', configuration: 'testClasses')

testImplementation libraries.guava
testImplementation project(':opentelemetry-sdk-testing')
testImplementation libraries.junit_pioneer

Expand Down
2 changes: 1 addition & 1 deletion sdk/trace/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ dependencies {
api project(':opentelemetry-api'),
project(':opentelemetry-sdk-common')

implementation libraries.guava

annotationProcessor libraries.auto_value

Expand All @@ -23,6 +22,7 @@ dependencies {

testCompile project(path: ':opentelemetry-sdk-common', configuration: 'testClasses')

testImplementation libraries.guava
testImplementation project(':opentelemetry-sdk-testing')
testImplementation libraries.junit_pioneer

Expand Down
5 changes: 2 additions & 3 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import static io.opentelemetry.api.common.AttributeKey.longKey;
import static io.opentelemetry.api.common.AttributeKey.stringKey;

import com.google.common.annotations.VisibleForTesting;
import io.opentelemetry.api.common.AttributeConsumer;
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -261,13 +260,13 @@ private static Clock getClock(Span parent, Clock clock) {
}
}

@VisibleForTesting
// Visible for testing
static boolean isRecording(SamplingResult.Decision decision) {
return SamplingResult.Decision.RECORD_ONLY.equals(decision)
|| SamplingResult.Decision.RECORD_AND_SAMPLE.equals(decision);
}

@VisibleForTesting
// Visible for testing
static boolean isSampled(SamplingResult.Decision decision) {
return SamplingResult.Decision.RECORD_AND_SAMPLE.equals(decision);
}
Expand Down
34 changes: 21 additions & 13 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/config/TraceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
package io.opentelemetry.sdk.trace.config;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import io.opentelemetry.api.internal.Utils;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.sdk.common.export.ConfigBuilder;
Expand Down Expand Up @@ -191,7 +189,7 @@ public abstract static class Builder extends ConfigBuilder<Builder> {
* @param configMap {@link Map} holding the configuration values.
* @return this
*/
@VisibleForTesting
// Visible for testing
@Override
protected Builder fromConfigMap(
Map<String, String> configMap, Builder.NamingConvention namingConvention) {
Expand Down Expand Up @@ -352,16 +350,26 @@ public Builder setTraceIdRatioBased(double samplerRatio) {
*/
public TraceConfig build() {
TraceConfig traceConfig = autoBuild();
Preconditions.checkArgument(
traceConfig.getMaxNumberOfAttributes() > 0, "maxNumberOfAttributes");
Preconditions.checkArgument(traceConfig.getMaxNumberOfEvents() > 0, "maxNumberOfEvents");
Preconditions.checkArgument(traceConfig.getMaxNumberOfLinks() > 0, "maxNumberOfLinks");
Preconditions.checkArgument(
traceConfig.getMaxNumberOfAttributesPerEvent() > 0, "maxNumberOfAttributesPerEvent");
Preconditions.checkArgument(
traceConfig.getMaxNumberOfAttributesPerLink() > 0, "maxNumberOfAttributesPerLink");
Preconditions.checkArgument(
traceConfig.getMaxLengthOfAttributeValues() >= -1, "maxLengthOfAttributeValues");
if (traceConfig.getMaxNumberOfAttributes() <= 0) {

@Oberon00 Oberon00 Nov 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Didn't we have some Utils.checkArgument function?

@anuraaga anuraaga Nov 27, 2020

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.

Yeah we have it in API - if we're ok with cross-referencing from other artifacts (guess it's ok) I'll use it

@jkwatson jkwatson Nov 30, 2020

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.

Yeah, the SDK uses API classes quite a bit (obviously because it has to implement it!). I think this is a fine usage, as long as it's not in a strictly "internal" package.

throw new IllegalArgumentException("maxNumberOfAttributes must be greater than 0");
}
if (traceConfig.getMaxNumberOfEvents() <= 0) {
throw new IllegalArgumentException("maxNumberOfEvents must be greater than 0");
}
if (traceConfig.getMaxNumberOfLinks() <= 0) {
throw new IllegalArgumentException("maxNumberOfLinks must be greater than 0");
}
if (traceConfig.getMaxNumberOfAttributesPerEvent() <= 0) {
throw new IllegalArgumentException("maxNumberOfAttributesPerEvent must be greater than 0");
}
if (traceConfig.getMaxNumberOfAttributesPerLink() <= 0) {
throw new IllegalArgumentException("maxNumberOfAttributesPerLink must be greater than 0");
}
if (traceConfig.getMaxLengthOfAttributeValues() < -1) {
throw new IllegalArgumentException(
"maxLengthOfAttributeValues must be -1 to "
+ "disable length restriction, or 0 or higher to enable length restriction");
}
return traceConfig;
}
}
Expand Down
26 changes: 15 additions & 11 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.sdk.trace.export;

import com.google.common.annotations.VisibleForTesting;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.common.Labels;
import io.opentelemetry.api.internal.Utils;
Expand Down Expand Up @@ -316,11 +315,16 @@ public static final class Builder extends ConfigBuilder<Builder> {
private static final String KEY_EXPORT_TIMEOUT_MILLIS = "otel.bsp.export.timeout.millis";
private static final String KEY_SAMPLED = "otel.bsp.export.sampled";

@VisibleForTesting static final long DEFAULT_SCHEDULE_DELAY_MILLIS = 5000;
@VisibleForTesting static final int DEFAULT_MAX_QUEUE_SIZE = 2048;
@VisibleForTesting static final int DEFAULT_MAX_EXPORT_BATCH_SIZE = 512;
@VisibleForTesting static final int DEFAULT_EXPORT_TIMEOUT_MILLIS = 30_000;
@VisibleForTesting static final boolean DEFAULT_EXPORT_ONLY_SAMPLED = true;
// Visible for testing
static final long DEFAULT_SCHEDULE_DELAY_MILLIS = 5000;
// Visible for testing
static final int DEFAULT_MAX_QUEUE_SIZE = 2048;
// Visible for testing
static final int DEFAULT_MAX_EXPORT_BATCH_SIZE = 512;
// Visible for testing
static final int DEFAULT_EXPORT_TIMEOUT_MILLIS = 30_000;
// Visible for testing
static final boolean DEFAULT_EXPORT_ONLY_SAMPLED = true;

private final SpanExporter spanExporter;
private long scheduleDelayMillis = DEFAULT_SCHEDULE_DELAY_MILLIS;
Expand Down Expand Up @@ -382,7 +386,7 @@ public Builder setExportOnlySampled(boolean exportOnlySampled) {
return this;
}

@VisibleForTesting
// Visible for testing
boolean getExportOnlySampled() {
return exportOnlySampled;
}
Expand All @@ -402,7 +406,7 @@ public Builder setScheduleDelayMillis(long scheduleDelayMillis) {
return this;
}

@VisibleForTesting
// Visible for testing
long getScheduleDelayMillis() {
return scheduleDelayMillis;
}
Expand All @@ -421,7 +425,7 @@ public Builder setExporterTimeoutMillis(int exporterTimeoutMillis) {
return this;
}

@VisibleForTesting
// Visible for testing
int getExporterTimeoutMillis() {
return exporterTimeoutMillis;
}
Expand All @@ -444,7 +448,7 @@ public Builder setMaxQueueSize(int maxQueueSize) {
return this;
}

@VisibleForTesting
// Visible for testing
int getMaxQueueSize() {
return maxQueueSize;
}
Expand All @@ -465,7 +469,7 @@ public Builder setMaxExportBatchSize(int maxExportBatchSize) {
return this;
}

@VisibleForTesting
// Visible for testing
int getMaxExportBatchSize() {
return maxExportBatchSize;
}
Expand Down
6 changes: 3 additions & 3 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/SimpleSpanProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.sdk.trace.export;

import com.google.common.annotations.VisibleForTesting;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.common.CompletableResultCode;
import io.opentelemetry.sdk.common.export.ConfigBuilder;
Expand Down Expand Up @@ -114,7 +113,8 @@ public static final class Builder extends ConfigBuilder<Builder> {

private static final String KEY_SAMPLED = "otel.ssp.export.sampled";

@VisibleForTesting static final boolean DEFAULT_EXPORT_ONLY_SAMPLED = true;
// Visible for testing
static final boolean DEFAULT_EXPORT_ONLY_SAMPLED = true;
private final SpanExporter spanExporter;
private boolean exportOnlySampled = DEFAULT_EXPORT_ONLY_SAMPLED;

Expand Down Expand Up @@ -157,7 +157,7 @@ public Builder setExportOnlySampled(boolean exportOnlySampled) {
return this;
}

@VisibleForTesting
// Visible for testing
boolean getExportOnlySampled() {
return exportOnlySampled;
}
Expand Down
5 changes: 3 additions & 2 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/samplers/TraceIdRatioBasedSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package io.opentelemetry.sdk.trace.samplers;

import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import io.opentelemetry.api.common.ReadableAttributes;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.TraceId;
Expand All @@ -29,7 +28,9 @@ abstract class TraceIdRatioBasedSampler implements Sampler {
TraceIdRatioBasedSampler() {}

static TraceIdRatioBasedSampler create(double ratio) {
Preconditions.checkArgument(ratio >= 0.0 && ratio <= 1.0, "ratio must be in range [0.0, 1.0]");
if (ratio < 0.0 || ratio > 1.0) {
throw new IllegalArgumentException("ratio must be in range [0.0, 1.0]");
}
long idUpperBound;
// Special case the limits, to avoid any possible issues with lack of precision across
// double/long boundaries. For probability == 0.0, we use Long.MIN_VALUE as this guarantees
Expand Down