-
Notifications
You must be signed in to change notification settings - Fork 885
Add experimental support for log extended attributes #7123
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
Changes from all commits
53d5439
ed799ef
ea2155b
b36e1ef
b24e012
9b3f2ac
f313fd8
99d6cf2
ff97464
f690101
2ff9076
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.api.incubator.common; | ||
|
||
import io.opentelemetry.api.common.AttributeKey; | ||
import io.opentelemetry.api.common.Attributes; | ||
import io.opentelemetry.api.common.AttributesBuilder; | ||
import io.opentelemetry.api.internal.ImmutableKeyValuePairs; | ||
import java.util.ArrayList; | ||
import java.util.Comparator; | ||
import javax.annotation.Nullable; | ||
import javax.annotation.concurrent.Immutable; | ||
|
||
@Immutable | ||
final class ArrayBackedExtendedAttributes | ||
extends ImmutableKeyValuePairs<ExtendedAttributeKey<?>, Object> implements ExtendedAttributes { | ||
|
||
// We only compare the key name, not type, when constructing, to allow deduping keys with the | ||
// same name but different type. | ||
private static final Comparator<ExtendedAttributeKey<?>> KEY_COMPARATOR_FOR_CONSTRUCTION = | ||
Comparator.comparing(ExtendedAttributeKey::getKey); | ||
|
||
static final ExtendedAttributes EMPTY = ExtendedAttributes.builder().build(); | ||
|
||
@Nullable private Attributes attributes; | ||
|
||
private ArrayBackedExtendedAttributes( | ||
Object[] data, Comparator<ExtendedAttributeKey<?>> keyComparator) { | ||
super(data, keyComparator); | ||
} | ||
|
||
/** | ||
* Only use this constructor if you can guarantee that the data has been de-duped, sorted by key | ||
* and contains no null values or null/empty keys. | ||
* | ||
* @param data the raw data | ||
*/ | ||
ArrayBackedExtendedAttributes(Object[] data) { | ||
super(data); | ||
} | ||
|
||
@Override | ||
public ExtendedAttributesBuilder toBuilder() { | ||
return new ArrayBackedExtendedAttributesBuilder(new ArrayList<>(data())); | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
@Override | ||
@Nullable | ||
public <T> T get(ExtendedAttributeKey<T> key) { | ||
return (T) super.get(key); | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
@Override | ||
public Attributes asAttributes() { | ||
if (attributes == null) { | ||
AttributesBuilder builder = Attributes.builder(); | ||
forEach( | ||
(extendedAttributeKey, value) -> { | ||
AttributeKey<Object> attributeKey = | ||
(AttributeKey<Object>) extendedAttributeKey.asAttributeKey(); | ||
if (attributeKey != null) { | ||
builder.put(attributeKey, value); | ||
} | ||
}); | ||
attributes = builder.build(); | ||
} | ||
breedx-splk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return attributes; | ||
} | ||
|
||
static ExtendedAttributes sortAndFilterToAttributes(Object... data) { | ||
// null out any empty keys or keys with null values | ||
// so they will then be removed by the sortAndFilter method. | ||
for (int i = 0; i < data.length; i += 2) { | ||
ExtendedAttributeKey<?> key = (ExtendedAttributeKey<?>) data[i]; | ||
if (key != null && key.getKey().isEmpty()) { | ||
data[i] = null; | ||
Check warning on line 81 in api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ArrayBackedExtendedAttributes.java
|
||
} | ||
} | ||
return new ArrayBackedExtendedAttributes(data, KEY_COMPARATOR_FOR_CONSTRUCTION); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.api.incubator.common; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.function.Predicate; | ||
|
||
class ArrayBackedExtendedAttributesBuilder implements ExtendedAttributesBuilder { | ||
private final List<Object> data; | ||
|
||
ArrayBackedExtendedAttributesBuilder() { | ||
data = new ArrayList<>(); | ||
} | ||
|
||
ArrayBackedExtendedAttributesBuilder(List<Object> data) { | ||
this.data = data; | ||
} | ||
|
||
@Override | ||
public ExtendedAttributes build() { | ||
// If only one key-value pair AND the entry hasn't been set to null (by #remove(AttributeKey<T>) | ||
// or #removeIf(Predicate<AttributeKey<?>>)), then we can bypass sorting and filtering | ||
if (data.size() == 2 && data.get(0) != null) { | ||
return new ArrayBackedExtendedAttributes(data.toArray()); | ||
} | ||
return ArrayBackedExtendedAttributes.sortAndFilterToAttributes(data.toArray()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sorting feels like part of the building, so what about moving the method into this builder class instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this whole function is essential a named constructor, since the constructor itself was private. For what its worth, this whole class is a "copy / paste / modify" of ArrayBackedAttributes, so I didn't look too critically at the implementation. What do you think about a followup PR where I cleanup minor pattern quirks like this in both ArrayBackedAttributes and ExtendedArrayBackedAttributes? |
||
} | ||
|
||
@Override | ||
public <T> ExtendedAttributesBuilder put(ExtendedAttributeKey<T> key, T value) { | ||
if (key == null || key.getKey().isEmpty() || value == null) { | ||
return this; | ||
Check warning on line 37 in api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ArrayBackedExtendedAttributesBuilder.java
|
||
} | ||
data.add(key); | ||
data.add(value); | ||
return this; | ||
} | ||
|
||
@Override | ||
public ExtendedAttributesBuilder removeIf(Predicate<ExtendedAttributeKey<?>> predicate) { | ||
if (predicate == null) { | ||
return this; | ||
Check warning on line 47 in api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ArrayBackedExtendedAttributesBuilder.java
|
||
} | ||
for (int i = 0; i < data.size() - 1; i += 2) { | ||
Object entry = data.get(i); | ||
if (entry instanceof ExtendedAttributeKey | ||
&& predicate.test((ExtendedAttributeKey<?>) entry)) { | ||
// null items are filtered out in ArrayBackedAttributes | ||
data.set(i, null); | ||
data.set(i + 1, null); | ||
} | ||
} | ||
return this; | ||
} | ||
|
||
static List<Double> toList(double... values) { | ||
Double[] boxed = new Double[values.length]; | ||
for (int i = 0; i < values.length; i++) { | ||
boxed[i] = values[i]; | ||
} | ||
return Arrays.asList(boxed); | ||
} | ||
|
||
static List<Long> toList(long... values) { | ||
Long[] boxed = new Long[values.length]; | ||
for (int i = 0; i < values.length; i++) { | ||
boxed[i] = values[i]; | ||
} | ||
return Arrays.asList(boxed); | ||
} | ||
|
||
static List<Boolean> toList(boolean... values) { | ||
Boolean[] boxed = new Boolean[values.length]; | ||
for (int i = 0; i < values.length; i++) { | ||
boxed[i] = values[i]; | ||
} | ||
return Arrays.asList(boxed); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.api.incubator.common; | ||
|
||
import io.opentelemetry.api.common.AttributeKey; | ||
import io.opentelemetry.api.incubator.internal.InternalExtendedAttributeKeyImpl; | ||
import java.util.List; | ||
import javax.annotation.Nullable; | ||
import javax.annotation.concurrent.Immutable; | ||
|
||
/** | ||
* This interface provides a handle for setting the values of {@link ExtendedAttributes}. The type | ||
* of value that can be set with an implementation of this key is denoted by the type parameter. | ||
* | ||
* <p>Implementations MUST be immutable, as these are used as the keys to Maps. | ||
* | ||
* <p>The allowed {@link #getType()}s is a superset of those allowed in {@link AttributeKey}. | ||
* | ||
* <p>Convenience methods are provided for translating to / from {@link AttributeKey}: | ||
* | ||
* <ul> | ||
* <li>{@link #asAttributeKey()} converts from {@link ExtendedAttributeKey} to {@link | ||
* AttributeKey} | ||
* <li>{@link #fromAttributeKey(AttributeKey)} converts from {@link AttributeKey} to {@link | ||
* ExtendedAttributeKey} | ||
* </ul> | ||
* | ||
* @param <T> The type of value that can be set with the key. | ||
*/ | ||
@Immutable | ||
public interface ExtendedAttributeKey<T> { | ||
/** Returns the underlying String representation of the key. */ | ||
String getKey(); | ||
|
||
/** Returns the type of attribute for this key. Useful for building switch statements. */ | ||
ExtendedAttributeType getType(); | ||
|
||
/** | ||
* Return the equivalent {@link AttributeKey}, or {@code null} if the {@link #getType()} has no | ||
* equivalent {@link io.opentelemetry.api.common.AttributeType}. | ||
*/ | ||
@Nullable | ||
default AttributeKey<T> asAttributeKey() { | ||
return InternalExtendedAttributeKeyImpl.toAttributeKey(this); | ||
Check warning on line 47 in api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ExtendedAttributeKey.java
|
||
} | ||
|
||
/** Return an ExtendedAttributeKey equivalent to the {@code attributeKey}. */ | ||
// TODO (jack-berg): remove once AttributeKey.asExtendedAttributeKey is available | ||
static <T> ExtendedAttributeKey<T> fromAttributeKey(AttributeKey<T> attributeKey) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per this comment, the conversion from AttributeKey -> ExtendedAttributeKey unfortunately lives on ExtendedAttributeKey instead of on AttributeKey. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That totally seems like the kind of thing that will take some usage to determine if/how annoying it might be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its preferable to live on Once its available on |
||
return InternalExtendedAttributeKeyImpl.toExtendedAttributeKey(attributeKey); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for String valued attributes. */ | ||
static ExtendedAttributeKey<String> stringKey(String key) { | ||
return fromAttributeKey(AttributeKey.stringKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for Boolean valued attributes. */ | ||
static ExtendedAttributeKey<Boolean> booleanKey(String key) { | ||
return fromAttributeKey(AttributeKey.booleanKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for Long valued attributes. */ | ||
static ExtendedAttributeKey<Long> longKey(String key) { | ||
return fromAttributeKey(AttributeKey.longKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for Double valued attributes. */ | ||
static ExtendedAttributeKey<Double> doubleKey(String key) { | ||
return fromAttributeKey(AttributeKey.doubleKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for List<String> valued attributes. */ | ||
static ExtendedAttributeKey<List<String>> stringArrayKey(String key) { | ||
return fromAttributeKey(AttributeKey.stringArrayKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for List<Boolean> valued attributes. */ | ||
static ExtendedAttributeKey<List<Boolean>> booleanArrayKey(String key) { | ||
return fromAttributeKey(AttributeKey.booleanArrayKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for List<Long> valued attributes. */ | ||
static ExtendedAttributeKey<List<Long>> longArrayKey(String key) { | ||
return fromAttributeKey(AttributeKey.longArrayKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for List<Double> valued attributes. */ | ||
static ExtendedAttributeKey<List<Double>> doubleArrayKey(String key) { | ||
return fromAttributeKey(AttributeKey.doubleArrayKey(key)); | ||
} | ||
|
||
/** Returns a new ExtendedAttributeKey for Map valued attributes. */ | ||
static ExtendedAttributeKey<ExtendedAttributes> extendedAttributesKey(String key) { | ||
return InternalExtendedAttributeKeyImpl.create(key, ExtendedAttributeType.EXTENDED_ATTRIBUTES); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.api.incubator.common; | ||
|
||
/** | ||
* An enum that represents all the possible value types for an {@link ExtendedAttributeKey} and | ||
* hence the types of values that are allowed for {@link ExtendedAttributes}. | ||
* | ||
* <p>This is a superset of {@link io.opentelemetry.api.common.AttributeType}, | ||
*/ | ||
public enum ExtendedAttributeType { | ||
// Types copied AttributeType | ||
STRING, | ||
BOOLEAN, | ||
LONG, | ||
DOUBLE, | ||
STRING_ARRAY, | ||
BOOLEAN_ARRAY, | ||
LONG_ARRAY, | ||
DOUBLE_ARRAY, | ||
// Extended types unique to ExtendedAttributes | ||
EXTENDED_ATTRIBUTES; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should we consider adding support for byte arrays? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it has to be now. Users will base64 encode or json stuff anyway. 🤷🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah users will always have that ability, but having direct representation for a type a signal that there is a recommended way of representing the data. This encourages more consistency than being opinionated, which will produce a bit of a free for all in terms of what instrumentations produce and what backends receive. Still, I'm happy to punt on these questions for now and let semantic conventions develop more guidance. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big loss. In the original prototype, I had a helper method to directly on
AttributeKey
for converting toExtendedAttributeKey
. This allowed theAttributeKey
to internally keep a cached copy of the correspondingExtendedAttributeKey
, making conversions in both directions very cheap.As long as
ExtendedAttributes
are experimental, we can't have this API. This means we need to a different strategy for converting / caching. I've opted for keeping a staticConcurrentHashMap
cache in InternalExtendedAttributeKey. The problem is that the map can grow unbounded if a user puts high cardinality data into attribute keys. Not as elegant as caching directly inAttributeKey
implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per a conversation in a java sig (several weeks ago I think), performance isn't a priority while this is experimental. Therefore, I've removed the unbounded cache for now.