opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

fix(api-logs,sdk-logs): allow AnyValue attributes for logs and handle circular references

Open Alec2435 opened this issue 5 months ago • 8 comments

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:

  • Byte arrays (Uint8Array)
  • Heterogeneous arrays (mixed types like [1, "two", true, null])
  • Nested objects/maps with recursive AnyValue support
  • Empty objects ({})
  • Null/undefined values

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 for null 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

  1. Created isLogAttributeValue() validation function in @opentelemetry/api-logs that supports all AnyValue types per the OpenTelemetry spec
  2. Added circular reference detection using WeakSet to prevent stack overflow during validation
  3. Updated LogRecordImpl to use the new spec-compliant validation function
  4. Enhanced truncation logic to handle nested objects and heterogeneous arrays
  5. Removed null value early return to allow spec-compliant null attributes
  6. Added comprehensive test coverage with 32+ tests covering all spec requirements and edge cases

Type of change

Please delete options that are not relevant.

  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

  • [x] Full test suite passes: All existing tests continue to pass (101 passing, 0 failing)
  • [x] New validation function tests: 32 comprehensive tests in validation.test.ts covering:
    • All scalar values (string, number, boolean)
    • Byte arrays (Uint8Array)
    • Heterogeneous arrays with mixed types
    • Nested objects and complex combinations
    • Circular reference detection and graceful handling
    • Invalid value rejection (functions, symbols, built-in objects)
    • Edge cases (deep nesting, large arrays)
  • [x] LogRecord integration tests: 15 new tests in LogRecord.test.ts verifying:
    • All AnyValue types work with setAttribute()
    • Proper truncation of strings in complex structures
    • Null/undefined value support
    • Complex nested attribute structures

Checklist:

  • [x] Followed the style guidelines of this project
  • [x] Unit tests have been added
  • [x] Documentation has been updated (via comprehensive test documentation and JSDoc comments)

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)

Alec2435 avatar Jun 25 '25 19:06 Alec2435

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: Alec2435 / name: Alec Velikanov (75f3d26fa35b2ffc052a7acdef4fcffce93cc6e3, cb0461a9391da16b5d09433aacf0037ea5eeae60, 5f795934e2decbdbc746344cc977d1a961889159)

@JacksonWeber are your comments addressed?

This is required by spec so I expect it to be uncontroversial to get this merged soon.

dyladan avatar Jul 16 '25 16:07 dyladan

@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.

JacksonWeber avatar Jul 21 '25 17:07 JacksonWeber

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

Alec2435 avatar Jul 21 '25 17:07 Alec2435

@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.

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.

dyladan avatar Jul 22 '25 17:07 dyladan

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 avatar Jul 22 '25 19:07 dyladan

@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 avatar Aug 04 '25 19:08 Alec2435

@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 :)

pichlermarc avatar Aug 20 '25 16:08 pichlermarc