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

add additional attributes to metric #5453

Closed
wants to merge 2 commits into from
Closed

add additional attributes to metric #5453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8681b7c
add additional attributes to metric
oliver-zhang May 15, 2023
ad3abe9
code format
oliver-zhang May 15, 2023
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
19 changes: 19 additions & 0 deletions .../io/opentelemetry/sdk/autoconfigure/spi/metrics/ConfigurableMetricAttributesProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure.spi.metrics;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;

/**
* A service provider interface (SPI) for providing additional attributes that can be used with the
* View AttributeProcessor.
*/
public interface ConfigurableMetricAttributesProvider {

/** Returns a {@link Attributes} that can be added to OpenTelemetry view AttributeProcessor */
Attributes addCustomAttributes(ConfigProperties config);
}
58 changes: 57 additions & 1 deletion .../src/main/java/io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,29 @@
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toList;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException;
import io.opentelemetry.sdk.autoconfigure.spi.metrics.ConfigurableMetricAttributesProvider;
import io.opentelemetry.sdk.metrics.Aggregation;
import io.opentelemetry.sdk.metrics.InstrumentSelector;
import io.opentelemetry.sdk.metrics.InstrumentSelectorBuilder;
import io.opentelemetry.sdk.metrics.InstrumentType;
import io.opentelemetry.sdk.metrics.SdkMeterProviderBuilder;
import io.opentelemetry.sdk.metrics.View;
import io.opentelemetry.sdk.metrics.ViewBuilder;
import io.opentelemetry.sdk.metrics.internal.SdkMeterProviderUtil;
import io.opentelemetry.sdk.metrics.internal.aggregator.AggregationUtil;
import io.opentelemetry.sdk.metrics.internal.view.AttributesProcessor;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.Set;
import javax.annotation.Nullable;
import org.snakeyaml.engine.v2.api.Load;
Expand Down Expand Up @@ -89,10 +96,24 @@ public static void registerViews(
for (ViewConfigSpecification viewConfigSpec : viewConfigSpecs) {
meterProviderBuilder.registerView(
toInstrumentSelector(viewConfigSpec.getSelectorSpecification()),
toView(viewConfigSpec.getViewSpecification()));
toView(viewConfigSpec.getViewSpecification(),null));
}
}

public static void registerViews(
SdkMeterProviderBuilder meterProviderBuilder, InputStream inputStream,
ConfigProperties configProperties) {
List<ViewConfigSpecification> viewConfigSpecs = loadViewConfig(inputStream);

for (ViewConfigSpecification viewConfigSpec : viewConfigSpecs) {
meterProviderBuilder.registerView(
toInstrumentSelector(viewConfigSpec.getSelectorSpecification()),
toView(viewConfigSpec.getViewSpecification(),configProperties));
}
}



// Visible for testing
@SuppressWarnings("unchecked")
static List<ViewConfigSpecification> loadViewConfig(InputStream inputStream) {
Expand Down Expand Up @@ -192,6 +213,41 @@ static View toView(ViewSpecification viewSpec) {
return builder.build();
}

static View toView(ViewSpecification viewSpec,@Nullable ConfigProperties configProperties) {
ViewBuilder builder = View.builder();
// add additional attributes
if (configProperties != null) {
AttributesBuilder attributesBuilder = Attributes.builder();
for (ConfigurableMetricAttributesProvider provider : ServiceLoader.load(
ConfigurableMetricAttributesProvider.class)){
attributesBuilder.putAll(provider.addCustomAttributes(configProperties));
}
SdkMeterProviderUtil.appendAdditionalAttributes(builder, AttributesProcessor.append(attributesBuilder.build()));
Copy link
Member

Choose a reason for hiding this comment

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

Since this is an extension to the YAML file configuration for views, I think it would make more sense to implement this as an additional section of key value pairs within the YAML, rather than an SPI that surprisingly interacts with all registered views.

With that said, I'm reluctant to add configurable surface area for adding these attributes since using views to add additional attributes is experimental and not an official part of the spec. That means that when we replace this file based YAML config with the full file configuration solution (currently being worked on), we'd have to drop the ability to append attributes via views.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is an extension to the YAML file configuration for views, I think it would make more sense to implement this as an additional section of key value pairs within the YAML, rather than an SPI that surprisingly interacts with all registered views.

With that said, I'm reluctant to add configurable surface area for adding these attributes since using views to add additional attributes is experimental and not an official part of the spec. That means that when we replace this file based YAML config with the full file configuration solution (currently being worked on), we'd have to drop the ability to append attributes via views.

Agree, My earliest consideration was the implementation you mentioned. Perhaps a specific metric needs to extend attributes,that's very suitable,Considering the common attributes of metrics, such as clusters, planes, etc., If each metric needs to be configured separately, it would be a lot of work,So I adopted the SPI. Do you have any good suggestions for the common attributes of all metrics, There maybe be better way. @jack-berg

Copy link
Member

Choose a reason for hiding this comment

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

Mentioned this in slack, but the most correct solution is probably to create a delegating MetricExporter which enriches MetricData with the additional attributes before calling a subsequent MetricExporter. While probably the most correct solution, this is awkward because it requires you to provide your own implementations of the MetricData interfaces. There's an issue tracking this here.

}
String name = viewSpec.getName();
if (name != null) {
builder.setName(name);
}
String description = viewSpec.getDescription();
if (description != null) {
builder.setDescription(description);
}
String aggregation = viewSpec.getAggregation();
if (aggregation != null) {
Map<String, Object> aggregationArgs =
viewSpec.getAggregationArgs() == null
? Collections.emptyMap()
: viewSpec.getAggregationArgs();
builder.setAggregation(toAggregation(aggregation, aggregationArgs));
}
List<String> attributeKeys = viewSpec.getAttributeKeys();
if (attributeKeys != null) {
Set<String> keySet = new HashSet<>(attributeKeys);
builder.setAttributeFilter(keySet::contains);
}
return builder.build();
}

// Visible for testing
static Aggregation toAggregation(String aggregationName, Map<String, Object> aggregationArgs) {
Aggregation aggregation;
Expand Down
14 changes: 12 additions & 2 deletions ...java/io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfigCustomizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ static SdkMeterProviderBuilder customizeMeterProvider(
SdkMeterProviderBuilder meterProviderBuilder, ConfigProperties configProperties) {
List<String> configFileLocations =
configProperties.getList("otel.experimental.metrics.view.config");
boolean additionAttrsEnable = configProperties.getBoolean("otel.experimental.metrics.view.additional.enable",true);
for (String configFileLocation : configFileLocations) {
if (configFileLocation.startsWith("classpath:")) {
String classpathLocation = configFileLocation.substring("classpath:".length());
Expand All @@ -41,15 +42,24 @@ static SdkMeterProviderBuilder customizeMeterProvider(
+ " not found on classpath of classloader "
+ ViewConfigCustomizer.class.getClassLoader().getClass().getName());
}
ViewConfig.registerViews(meterProviderBuilder, inputStream);
if (additionAttrsEnable) {
ViewConfig.registerViews(meterProviderBuilder,inputStream,configProperties);
} else {
ViewConfig.registerViews(meterProviderBuilder, inputStream);
}

} catch (IOException e) {
throw new ConfigurationException(
"An error occurred reading view config resource on classpath: " + classpathLocation,
e);
}
} else {
try (FileInputStream fileInputStream = new FileInputStream(configFileLocation)) {
ViewConfig.registerViews(meterProviderBuilder, fileInputStream);
if (additionAttrsEnable) {
ViewConfig.registerViews(meterProviderBuilder,fileInputStream,configProperties);
} else {
ViewConfig.registerViews(meterProviderBuilder, fileInputStream);
}
} catch (FileNotFoundException e) {
throw new ConfigurationException("View config file not found: " + configFileLocation, e);
} catch (IOException e) {
Expand Down
14 changes: 14 additions & 0 deletions ...metry/sdk/extension/incubator/metric/viewconfig/CustomConfigurableAttributesProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package io.opentelemetry.sdk.extension.incubator.metric.viewconfig;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.autoconfigure.spi.metrics.ConfigurableMetricAttributesProvider;


public class CustomConfigurableAttributesProvider implements ConfigurableMetricAttributesProvider {
@Override
public Attributes addCustomAttributes(ConfigProperties config) {
return Attributes.of(AttributeKey.stringKey("foo"),"val");
}
}
59 changes: 59 additions & 0 deletions .../io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfigAdditionalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package io.opentelemetry.sdk.extension.incubator.metric.viewconfig;

import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
import static org.assertj.core.api.Assertions.assertThat;

import com.google.common.collect.ImmutableMap;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import org.junit.jupiter.api.Test;

public class ViewConfigAdditionalTest {

@Test
public void testViewConfigAdditionAttrs() {
InMemoryMetricReader reader = InMemoryMetricReader.create();
AutoConfiguredOpenTelemetrySdk.builder()
.setResultAsGlobal(false)
.addPropertiesSupplier(
() ->
ImmutableMap.of(
"otel.traces.exporter",
"none",
"otel.metrics.exporter",
"none",
"otel.experimental.metrics.view.config",
"classpath:/view-config-customizer-test.yaml"))
.addMeterProviderCustomizer(
(meterProviderBuilder, configProperties) ->
meterProviderBuilder.registerMetricReader(reader))
.build()
.getOpenTelemetrySdk()
.getSdkMeterProvider()
.get("test-meter")
.counterBuilder("counter")
.buildWithCallback(
callback -> {
// Attributes with keys baz and qux should be filtered out
callback.record(
1,
Attributes.builder()
.build());
});

assertThat(reader.collectAllMetrics())
.satisfiesExactly(
metricData ->
OpenTelemetryAssertions.assertThat(metricData)
.hasLongSumSatisfying(
sum ->
sum.hasPointsSatisfying(
point ->
point
.hasValue(1)
.hasAttributes(
attributeEntry("foo", "val")))));
}
}
1 change: 1 addition & 0 deletions ...vices/io.opentelemetry.sdk.autoconfigure.spi.metrics.ConfigurableMetricAttributesProvider
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
io.opentelemetry.sdk.extension.incubator.metric.viewconfig.CustomConfigurableAttributesProvider
5 changes: 5 additions & 0 deletions sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/SdkMeterProviderUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public static void appendAllBaggageAttributes(ViewBuilder viewBuilder) {
appendFilteredBaggageAttributes(viewBuilder, StringPredicates.ALL);
}

public static void appendAdditionalAttributes(
ViewBuilder viewBuilder, AttributesProcessor attributesProcessor) {
addAttributesProcessor(viewBuilder, attributesProcessor);
}

private static void addAttributesProcessor(
ViewBuilder viewBuilder, AttributesProcessor attributesProcessor) {
try {
Expand Down