-
Notifications
You must be signed in to change notification settings - Fork 916
Complex attributes incubating implementation #7814
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
base: main
Are you sure you want to change the base?
Conversation
f91ab1f to
6e1c6e7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7814 +/- ##
============================================
+ Coverage 90.16% 90.17% +0.01%
- Complexity 7229 7321 +92
============================================
Files 821 822 +1
Lines 21809 22007 +198
Branches 2136 2180 +44
============================================
+ Hits 19664 19845 +181
- Misses 1476 1488 +12
- Partials 669 674 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cbdcc32 to
2b69000
Compare
4da140a to
26a3413
Compare
api/all/src/main/java/io/opentelemetry/api/common/ArrayBackedAttributesBuilder.java
Outdated
Show resolved
Hide resolved
jkwatson
left a comment
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.
Anyone who wants to use the Value type should be scared off by looking at the complexity of the implementation. 😂 Thanks!
|
I was kinda hoping we could merge this right into the stable |
|
we can't merge it into stable until Jan 15 (6 months after the OTEP was merged) |
|
TODO: implement attribute limits |
Is there consensus on how to do this? |
|
f27638a to
4386c8c
Compare
| return asValue(currentKey.getType(), value); | ||
| } | ||
| } | ||
| return null; |
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.
Would this implementation benefit from #7076?
| switch (valueObj.getType()) { | ||
| case STRING: | ||
| data.add(stringKey(keyName)); | ||
| data.add(valueObj.getValue()); |
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.
Can use a pattern like: put(stringKey(keyName), ((Value<String>) valueObj).getValue()) to save a few lines.
Is there a way to be able to store the value key without having to convert it to a string / long / double / etc key first? I.e. something like:
- Always store the value key
- When fetching for a given key, say of type string, check:
- If there is a matching key with type string
- OR if there is a matching key of type value and with a value of type string
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.
Can use a pattern like:
put(stringKey(keyName), ((Value<String>) valueObj).getValue())to save a few lines.
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.
Is there a way to be able to store the value key without having to convert it to a string / long / double / etc key first? I.e. something like:
Always store the value key
When fetching for a given key, say of type string, check:
- If there is a matching key with type string
- OR if there is a matching key of type value and with a value of type string
I think it's easier to store them in the format we want to expose them via asMap() and forEach()
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.
Ok, but there's obviously a steep perf penalty for making a copy of the attribute key every time. Maybe that's fine as it provides a disincentive for using VALUE where not necessary. Could also solve with an optimization in ExtendedAttributeKey, where we precompute and/or cache the primitive attribute key equivalent when the type is VALUE. Then, this code could leverage the preocmputed / cached value.
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.
Ok, but there's obviously a steep perf penalty for making a copy of the attribute key every time [you use a Value attribute to wrap a simple attribute]
About the same perf penalty as wrapping the simple attribute in a Value.of()?
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.
Yes. Steep is a bit hyperbolic, but these apis could end up on the hot path so maybe not
...rc/main/java/io/opentelemetry/api/incubator/common/ArrayBackedExtendedAttributesBuilder.java
Outdated
Show resolved
Hide resolved
| if (isArrayType(key.getType())) { | ||
| T value = (T) super.get(key); | ||
| if (value == null) { | ||
| // Check if there's a VALUE with the same key that contains an empty array |
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.
I don't understand this bit. Is there special handling of empty somewhere?
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 for
* <p>Further, if {@code put(ExtendedAttributeKey.valueKey("key"), Value.of(emptyList()))} was
* called, then
*
* <ul>
* <li>{@code get(ExtendedAttributeKey.stringArrayKey("key"))}
* <li>{@code get(ExtendedAttributeKey.longArrayKey("key"))}
* <li>{@code get(ExtendedAttributeKey.booleanArrayKey("key"))}
* <li>{@code get(ExtendedAttributeKey.doubleArrayKey("key"))}
* </ul>
*
* <p>all return an empty list (as opposed to {@code null}).
tested here:
@Test
void emptyValueArrayRetrievedAsAnyArrayType() {
ExtendedAttributes attributes =
ExtendedAttributes.builder()
.put(valueKey("key"), Value.of(Collections.emptyList()))
.build();
assertThat(attributes.get(stringArrayKey("key"))).isEmpty();
assertThat(attributes.get(longArrayKey("key"))).isEmpty();
assertThat(attributes.get(doubleArrayKey("key"))).isEmpty();
assertThat(attributes.get(booleanArrayKey("key"))).isEmpty();
}
| switch (attributeType) { | ||
| case STRING_ARRAY: | ||
| List<String> strings = new ArrayList<>(); | ||
| for (Value<?> v : arrayValues) { |
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.
If we're interested in premature performance optimization, there's probably a way to to merge this with attributeType function to iterate only once.
I'd say only pursue if it ends up making the code more concise with the perf improvement.
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.
I think it would require optimistically creating the array as we loop through and potentially throwing it away?
Based on prototype option C (#7813)