PoC: widen 'Attributes' to support the extended AnyValue#6579
Conversation
|
For reference OTel Spec 1.49.0 includes:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md#v1490-2025-09-16 |
… all of this needs to be part of *this* PR work
…ibuteValue (now deprecated) because AnyValue is in the spec for more than just attributes (e.g. LogRecord#body)
…d attributes in sdk-metrics (keyObjFromAnyValue util in core package)
trentm
left a comment
There was a problem hiding this comment.
Add some notes to self, and for reviews.
| * @param [options] the metric options. | ||
| */ | ||
| createGauge<AttributesTypes extends MetricAttributes = MetricAttributes>( | ||
| createGauge<AttributesTypes extends Attributes = Attributes>( |
There was a problem hiding this comment.
Q: What is the <AttributesTypes extends ...> generic on Instruments for?
| * @param [options] the metric options. | ||
| */ | ||
| createHistogram<AttributesTypes extends MetricAttributes = MetricAttributes>( | ||
| createHistogram<AttributesTypes extends Attributes = Attributes>( |
There was a problem hiding this comment.
There are a lot of s/{Metric,Span}Attributes/Attributes/ changes in the PR that could be done in a separate PR to make it smaller. I may end up separating them to help reviews.
| attributes?: Attributes; | ||
| // XXX A bit silly/incorrect to have this dropped count on the `Link` type | ||
| // used by Span#addLink. The user is not meant to specify this count. | ||
| // We could deprecate this, and then cope in the SDK. |
There was a problem hiding this comment.
This is totally for a separate PR. This is a note to self.
| // TODO: Remove ProxyTracerProvider export in the next major version. | ||
| export { ProxyTracerProvider } from './trace/ProxyTracerProvider'; | ||
| export type { Sampler } from './trace/Sampler'; | ||
| export { SamplingDecision, type SamplingResult } from './trace/SamplingResult'; |
There was a problem hiding this comment.
This belongs in a separate PR, if at all. I did this as I was working through the full API to grok usage of Attributes/AnyValue.
| return isAttributeValueInternal(val, new WeakSet()); | ||
| } | ||
|
|
||
| function isAttributeValueInternal( |
There was a problem hiding this comment.
This impl is mostly from the current sdk-logs, e.g.: isLogAttributeValueInternal().
| // XXX Want a version without limits: resources and metrics are exempt: https://opentelemetry.io/docs/specs/otel/common/#exempt-entities | ||
| // XXX Having to pass around attribute count (currentAttributesCount, etc.) is a pain. Not suggesting it now, because more work, but using a Map would be nice for the API. | ||
| // XXX Consider moving diag.warn()s out to caller. | ||
| export function addAttribute( |
There was a problem hiding this comment.
| // XXX use normalizeAttributes, keep droppedAttributeCount values, | ||
| // separate from the `links` sent to shouldSample. Then pass links | ||
| // *with* droppedAttributeCount to SpanImpl, which accepts those. | ||
| // I.e. SpanImpl type shoudl be `Array<Link & {droppedAttributeCount?: number}>` |
There was a problem hiding this comment.
This belongs in a separate PR.
| // XXX Should this be defensively dropping attributes with invalid type? | ||
|
|
||
| const keyObj = keyObjFromAnyValue(attributes); | ||
| return JSON.stringify(keyObj); |
There was a problem hiding this comment.
I think this hashAttributes is the only "handling" of Attributes that is need to update sdk-metrics to support complex attributes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6579 +/- ##
==========================================
- Coverage 95.74% 89.51% -6.24%
==========================================
Files 375 84 -291
Lines 12704 1994 -10710
Branches 2998 417 -2581
==========================================
- Hits 12164 1785 -10379
+ Misses 540 209 -331
🚀 New features to boost your workflow:
|
|
instr-http tests are failing on a deepStrictEqual of span.attributes where a span's attributes include: We will need to discuss whether we actually want to allow the The spec https://opentelemetry.io/docs/specs/otel/common/#anyvalue says:
But my guess is this wasn't fully vetted. |
|
I mentioned that there's some ways to demonstrate why this is a breaking change. This is the type of code that compiles with the previous attribute type but breaks with the new one: /**
* A function that may validate or sanitize attributes before they are used in telemetry.
*/
function doStuffWithAttributes(attributes: Attributes) {
for (const attrName in attributes) {
const value = attributes[attrName];
if (
value == null ||
typeof value === 'string' ||
typeof value === 'number' ||
typeof value === 'boolean'
) {
// do whatever
continue;
}
// tsc now knows that this can now only be an Array,
// so it'll allow things that are available on Arrays.
// In the future, this may be an object so it will
// - break during compile time. (TS2723: Cannot invoke an object which is possibly null or undefined)
// - break during runtime for already compiled code that caret-depends on the API (like instrumentations that receive attributes from a user-exposed hook)
//
// I think all other possible examples where are essentially variations of this one.
//
// SDK components should be safe since they should specify an upper limit for supported API versions.
// Unfortunately, instrumentations are not bound by that.
return value.filter(item => typeof item == 'string');
}
} |
Refs: #6349
Current status
This is a hacked-up proof of concept, to explore whether support for complex/extended attributes could be accomplished without a separate attributes type in the API.
Can we widen
interface Attributes, and update the SDK package's usage of attributes?Code-wise, I think this change so far shows: yes.
sanitizeAttributes()already in the SDK packages now safely handle incoming attributes. The SDK was already necessarily defensive in checking the given attribute keys and values for validity.)