-
Notifications
You must be signed in to change notification settings - Fork 804
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
fix(api-logs): align AnyValue to spec #4893
Conversation
|
||
/** | ||
* AnyValueMap is a map from string to AnyValue (attribute value or a nested map) | ||
*/ | ||
export interface AnyValueMap { | ||
[attributeKey: string]: AnyValue | undefined; |
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.
reviewer note: since now AnyValue
can be null
or undefined
, it is not needed to state it here for map specifically
*/ | ||
export type AnyValue = AttributeValue | AnyValueMap; |
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.
reviewer note: Since AttributeValue
is:
export type AttributeValue =
| string
| number
| boolean
| Array<null | undefined | string>
| Array<null | undefined | number>
| Array<null | undefined | boolean>;
Then removing it and adding the AnyValueScalar
and AnyValueArray
should already include all the previous allowed type, thus this type only extends the previous one and thus I don't think it's a breaking change
export type AnyValue = AttributeValue | AnyValueMap; | ||
export type AnyValue = | ||
| AnyValueScalar | ||
| Uint8Array |
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.
There are several typed array types, do we want to have them all listed here? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray Maybe having a AnyTypedArray type now
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 any of the other TypedArrays would get borked on OTLP export. The toAnyValue()
in otlp-transformer specifically knows about Uint8Array:
if (value instanceof Uint8Array) return { bytesValue: value }; |
But doesn't know the other TypeArrays.
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 followed what the spec wording that said A byte array,
as something that should be supported as any value.
Since byte is 0-255, I think it aligns with Uint8 in contrast to Int8Array
for example, which semantically is not byte array I think. as @trentm mentioned, they cannot be serialized into OTLP as bytesValue
anyhow as the proto field only accepts bytes bytes_value = 7;
.
However, it will still be exported (to my understanding) as arrayValue
if (Array.isArray(value))
return { arrayValue: { values: value.map(toAnyValue) } };
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 will be exported as an object with integer-string keys, because:
> a = new Int8Array([1,2,3])
Int8Array(3) [ 1, 2, 3 ]
> Array.isArray(a)
false
> typeof a
'object'
From my playing around on open-telemetry/opentelemetry-js-contrib#2352 (comment) and with a LogRecord with an attribute aInt8Array: new Int8Array([-7,8,9])
the OTLP server sees something like this:
KeyValue {
key: 'aInt8Array',
value: AnyValue {
kvlistValue: KeyValueList {
values: [
KeyValue { key: '0', value: AnyValue { intValue: Long { low: -7, high: -1, unsigned: false } } },
KeyValue { key: '1', value: AnyValue { intValue: Long { low: 8, high: 0, unsigned: false } } },
KeyValue { key: '2', value: AnyValue { intValue: Long { low: 9, high: 0, unsigned: false } } }
]
}
}
},
FWIW, this is basically how JSON.stringify
also serializes these TypedArrays:
> JSON.stringify(a)
'{"0":1,"1":2,"2":3}'
@@ -14,16 +14,29 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import { AttributeValue } from '@opentelemetry/api'; | |||
export type AnyValueScalar = string | number | boolean; |
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.
We should support bigint here as well
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.
https://opentelemetry.io/docs/specs/otel/logs/data-model/#type-any says:
signed 64 bit integer,
but JS's MAX_SAFE_INTEGER
is less than that...
so, probably, yes we should add BigInt?
However BigInt requires Node.js 10.4.0 and api/package.json (the package in which api-logs eventually will land) has:
"engines": {
"node": ">=8.0.0"
},
so, I'm not sure if that is a problem.
There was some discussion around BigInt usage in some exporters-related PR a while back I thought.
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.
However, this could be discussed separate from this PR. This change isn't removing BigInt from the type.
@@ -14,16 +14,29 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import { AttributeValue } from '@opentelemetry/api'; | |||
export type AnyValueScalar = string | number | boolean; |
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.
https://opentelemetry.io/docs/specs/otel/logs/data-model/#type-any says:
signed 64 bit integer,
but JS's MAX_SAFE_INTEGER
is less than that...
so, probably, yes we should add BigInt?
However BigInt requires Node.js 10.4.0 and api/package.json (the package in which api-logs eventually will land) has:
"engines": {
"node": ">=8.0.0"
},
so, I'm not sure if that is a problem.
There was some discussion around BigInt usage in some exporters-related PR a while back I thought.
@@ -14,16 +14,29 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import { AttributeValue } from '@opentelemetry/api'; | |||
export type AnyValueScalar = string | number | boolean; |
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.
However, this could be discussed separate from this PR. This change isn't removing BigInt from the type.
export type AnyValue = AttributeValue | AnyValueMap; | ||
export type AnyValue = | ||
| AnyValueScalar | ||
| Uint8Array |
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 any of the other TypedArrays would get borked on OTLP export. The toAnyValue()
in otlp-transformer specifically knows about Uint8Array:
if (value instanceof Uint8Array) return { bytesValue: value }; |
But doesn't know the other TypeArrays.
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.
LGTM
Following this PR in contrib I noticed that the
AnyValue
type in@opentelemetry/api-logs
is not accurate, as the spec defined more allowed types. This PR aligns theAnyValue
type to match the spec: