-
Notifications
You must be signed in to change notification settings - Fork 936
fix(api-logs,sdk-logs): allow AnyValue attributes for logs and handle circular references #5765
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
@JacksonWeber are your comments addressed? This is required by spec so I expect it to be uncontroversial to get this merged soon. |
Updated coverage looks good, however I don't seem to have the ability to resolve my comments. |
Sorry for not shooting an update with my updated commit, I think the test coverage should be pretty comprehensive now, hoping we can get this merged for proper spec compliance. I think I've resolved your comments @JacksonWeber |
An unfortunate reality of github. You need write permissions of some kind to resolve threads unfortunately. I think the PR author usually can do it though. |
Do you think it would be reasonable to add the validation to the SDK? In general we try to keep as much logic in the SDK as possible in order to keep the API lightweight. Is there something in the spec that requires it in the API? |
@dyladan do you mean move the validation util to the sdk-logs package? Sure I can move it there, wasn't sure the best place to put it just saw the other type definitions were in api so thought it made sense to put the validation helper there. |
@Alec2435 yes, the SDK packge is the best place to put this. We try to avoid having business logic in the api package if at all possible :) |
export type { LogAttributes, LogBody, LogRecord } from './types/LogRecord'; | ||
export type { LoggerOptions } from './types/LoggerOptions'; | ||
export type { AnyValue, AnyValueMap } from './types/AnyValue'; | ||
export { isLogAttributeValue } from './utils/validation'; |
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 implementation code and should therefore go into sdk-logs
instead of here. The API is mostly intended for just type definitions and registering the global, anything else should go into sdk-logs
:)
Make Log Attributes fully spec compliant and handle circular references
Which problem is this PR solving?
The current Log Attributes implementation is not fully compliant with the OpenTelemetry specification. The specification states that Log Attributes should support "any type, a superset of standard Attribute" including:
Uint8Array
)[1, "two", true, null]
)AnyValue
support{}
)However, the current implementation uses the restrictive
isAttributeValue()
function from@opentelemetry/core
which only supports homogeneous arrays and primitives. Additionally, there was an early return fornull
values preventing spec-compliant null attributes from being set.This PR also addresses the circular reference investigation requested in the logs roadmap by implementing proper circular reference detection and handling.
Fixes #5744
Short description of the changes
isLogAttributeValue()
validation function in@opentelemetry/api-logs
that supports allAnyValue
types per the OpenTelemetry specWeakSet
to prevent stack overflow during validationLogRecordImpl
to use the new spec-compliant validation functionType of change
Please delete options that are not relevant.
How Has This Been Tested?
validation.test.ts
covering:Uint8Array
)LogRecord.test.ts
verifying:AnyValue
types work withsetAttribute()
Checklist:
Files changed:
experimental/packages/api-logs/src/utils/validation.ts
(new)experimental/packages/api-logs/src/index.ts
(export added)experimental/packages/api-logs/test/common/utils/validation.test.ts
(new)experimental/packages/sdk-logs/src/LogRecordImpl.ts
(updated)experimental/packages/sdk-logs/test/common/LogRecord.test.ts
(tests added)experimental/packages/sdk-logs/test/common/utils.ts
(updated for new behavior)