Skip to content

fix: don't modify the configuration in the Metrics constructor #411

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
cvas22 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: don't modify the configuration in the Metrics constructor #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 21 additions & 11 deletions src/main/java/dev/openfga/sdk/telemetry/Attributes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import dev.openfga.sdk.api.client.ApiResponse;
import dev.openfga.sdk.api.configuration.Configuration;
import dev.openfga.sdk.api.configuration.TelemetryConfiguration;
import dev.openfga.sdk.api.configuration.Credentials;
import dev.openfga.sdk.api.configuration.CredentialsMethod;
import io.opentelemetry.api.common.AttributeKey;
Expand Down Expand Up @@ -97,23 +98,32 @@ public class Attributes {
*/
public static io.opentelemetry.api.common.Attributes prepare(
Map<Attribute, String> attributes, Metric metric, Configuration configuration) {
return prepare(attributes, metric, configuration == null ? null : configuration.getTelemetryConfiguration());
}

/**
* Filters the given attributes based on the telemetry configuration.
*
* @param attributes the attributes to filter
* @param metric the metric to filter the attributes for
* @param telemetryConfiguration the telemetry configuration to filter with
*
* @return the prepared attributes
*/
public static io.opentelemetry.api.common.Attributes prepare(
Map<Attribute, String> attributes, Metric metric, TelemetryConfiguration telemetryConfiguration) {
if (attributes == null
|| attributes.isEmpty()
|| configuration == null
|| configuration.getTelemetryConfiguration() == null
|| configuration.getTelemetryConfiguration().metrics() == null
|| configuration.getTelemetryConfiguration().metrics().isEmpty()
|| !configuration.getTelemetryConfiguration().metrics().containsKey(metric)
|| configuration
.getTelemetryConfiguration()
.metrics()
.get(metric)
.isEmpty()) {
|| telemetryConfiguration == null
|| telemetryConfiguration.metrics() == null
|| telemetryConfiguration.metrics().isEmpty()
|| !telemetryConfiguration.metrics().containsKey(metric)
|| telemetryConfiguration.metrics().get(metric).isEmpty()) {
return io.opentelemetry.api.common.Attributes.empty();
}

Map<Attribute, Optional<Object>> configAllowedAttributes =
configuration.getTelemetryConfiguration().metrics().get(metric);
telemetryConfiguration.metrics().get(metric);

io.opentelemetry.api.common.AttributesBuilder builder = io.opentelemetry.api.common.Attributes.builder();

Expand Down
28 changes: 19 additions & 9 deletions src/main/java/dev/openfga/sdk/telemetry/Metrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public class Metrics {
private final Map<String, LongCounter> counters;
private final Map<String, DoubleHistogram> histograms;
private final Configuration configuration;
private final TelemetryConfiguration defaultTelemetryConfiguration;

public Metrics() {
this(new Configuration());
Expand All @@ -27,9 +28,16 @@ public Metrics(Configuration configuration) {
this.counters = new ConcurrentHashMap<>();
this.histograms = new ConcurrentHashMap<>();
this.configuration = configuration;
if (this.configuration.getTelemetryConfiguration() == null) {
this.configuration.telemetryConfiguration(new TelemetryConfiguration());
}
this.defaultTelemetryConfiguration = new TelemetryConfiguration();
}

/**
* Returns the telemetry configuration of the configuration, or the default one if it has none. The caller's
* configuration is never modified, since it may be shared.
*/
private TelemetryConfiguration telemetryConfiguration() {
TelemetryConfiguration telemetryConfiguration = configuration.getTelemetryConfiguration();
return telemetryConfiguration != null ? telemetryConfiguration : defaultTelemetryConfiguration;
}

/**
Expand All @@ -51,16 +59,17 @@ public Meter getMeter() {
* @return The LongCounter metric instance, if the counter was configured. Otherwise, null.
*/
public LongCounter getCounter(Counter counter, Long value, Map<Attribute, String> attributes) {
if (configuration.getTelemetryConfiguration().metrics() == null
|| !configuration.getTelemetryConfiguration().metrics().containsKey(counter)) {
TelemetryConfiguration telemetryConfiguration = telemetryConfiguration();
if (telemetryConfiguration.metrics() == null
|| !telemetryConfiguration.metrics().containsKey(counter)) {
return null;
}
LongCounter counterInstance = counters.computeIfAbsent(counter.getName(), name -> meter.counterBuilder(name)
.setDescription(counter.getDescription())
.build());

if (value != null) {
counterInstance.add(value, Attributes.prepare(attributes, counter, configuration));
counterInstance.add(value, Attributes.prepare(attributes, counter, telemetryConfiguration));
}

return counterInstance;
Expand All @@ -76,8 +85,9 @@ public LongCounter getCounter(Counter counter, Long value, Map<Attribute, String
* @return the DoubleHistogram instance, if the histogram was configured. Otherwise, null.
*/
public DoubleHistogram getHistogram(Histogram histogram, Double value, Map<Attribute, String> attributes) {
if (configuration.getTelemetryConfiguration().metrics() == null
|| !configuration.getTelemetryConfiguration().metrics().containsKey(histogram)) {
TelemetryConfiguration telemetryConfiguration = telemetryConfiguration();
if (telemetryConfiguration.metrics() == null
|| !telemetryConfiguration.metrics().containsKey(histogram)) {
return null;
}

Expand All @@ -88,7 +98,7 @@ public DoubleHistogram getHistogram(Histogram histogram, Double value, Map<Attri
.build());

if (value != null) {
histogramInstance.record(value, Attributes.prepare(attributes, histogram, configuration));
histogramInstance.record(value, Attributes.prepare(attributes, histogram, telemetryConfiguration));
}

return histogramInstance;
Expand Down
28 changes: 28 additions & 0 deletions src/test/java/dev/openfga/sdk/telemetry/MetricsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,34 @@ void shouldConstructorWithoutConfiguration() {
assertThat(metricsWithoutConfig.getMeter()).isNotNull();
}

@Test
void shouldNotModifyTheConfiguration() {
// given
Configuration configuration = new Configuration();
TelemetryConfiguration before = configuration.getTelemetryConfiguration();

// when
new Metrics(configuration).credentialsRequest(1L, Map.of());

// then
assertThat(configuration.getTelemetryConfiguration()).isSameAs(before);
}

@Test
void shouldUseTheDefaultTelemetryConfigurationWhenTheConfigurationHasNone() {
// given
Configuration configuration = new Configuration();
configuration.telemetryConfiguration(null);
Metrics metrics = new Metrics(configuration);

// when
LongCounter longCounter = metrics.credentialsRequest(1L, Map.of());

// then
assertThat(longCounter).isNotNull();
assertThat(configuration.getTelemetryConfiguration()).isNull();
}

@Test
void shouldGetMeter() {
// given
Expand Down