-
Notifications
You must be signed in to change notification settings - Fork 946
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])AnyValuesupport{})However, the current implementation uses the restrictive
isAttributeValue()function from@opentelemetry/corewhich only supports homogeneous arrays and primitives. Additionally, there was an early return fornullvalues 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-logsthat supports allAnyValuetypes per the OpenTelemetry specWeakSetto prevent stack overflow during validationLogRecordImplto use the new spec-compliant validation functionType of change
Please delete options that are not relevant.
How Has This Been Tested?
validation.test.tscovering:Uint8Array)LogRecord.test.tsverifying:AnyValuetypes 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)