Skip to content

Conversation

43081j
Copy link

@43081j 43081j commented Aug 20, 2025

Does a couple of things:

  • Switches to a regular for...in which is much faster than
    Object.entries
  • Only retrieves the val if the key is valid
  • Compares keys against "" (empty string) rather than accessing
    length
  • Reuses typeof result rather than recomputing it each time

Type of change

Performance improvement

How Has This Been Tested?

Existing unit tests cover this code, and pass successfully.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Does a couple of things:

- Switches to a regular `for...in` which is much faster than
  `Object.entries`
- Only retrieves the `val` if the key is valid
- Compares keys against `""` (empty string) rather than accessing
  `length`
- Reuses `typeof` result rather than recomputing it each time
@43081j 43081j requested a review from a team as a code owner August 20, 2025 14:44
Copy link

linux-foundation-easycla bot commented Aug 20, 2025

CLA Signed

  • ✅login: 43081j / name: James Garbutt / (dc7e48c)

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Aug 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.04%. Comparing base (8ffc70d) to head (dc7e48c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5866   +/-   ##
=======================================
  Coverage   95.04%   95.04%           
=======================================
  Files         307      307           
  Lines        7987     7991    +4     
  Branches     1614     1616    +2     
=======================================
+ Hits         7591     7595    +4     
  Misses        396      396           
Files with missing lines Coverage Δ
...ckages/opentelemetry-core/src/common/attributes.ts 93.75% <100.00%> (+0.56%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@raphael-theriault-swi
Copy link
Member

Could you provide some comparative figures given this is for perf ?

@43081j
Copy link
Author

43081j commented Aug 20, 2025

here's the results from a quick benchmark locally:

benchmark                   avg (min … max) p75 / p99    (min … top 1%)
------------------------------------------- -------------------------------
sanitizeAttributes           114.91 ns/iter 115.67 ns   ▅█
                    (109.01 ns … 186.76 ns) 135.03 ns   ██
                    (110.75  b … 611.93  b) 400.31  b ▁▁███▃▃▄▅▃▂▂▂▂▁▁▁▁▁▁▁

sanitizeAttributesMain       154.84 ns/iter 156.72 ns  █
                    (150.32 ns … 251.54 ns) 170.29 ns  █▃
                    (821.69  b …   2.59 kb)   1.00 kb ▆██▄▃▃▄▄▃▂▂▃▃▂▁▂▁▁▁▁▁

summary
  sanitizeAttributes
   1.35x faster than sanitizeAttributesMain

to be honest though, a large amount of the time is lost to warn calls 🤔

i wonder if you could optimise the log proxy to compute the logger up front?

basically this:

      const logger = getGlobal('diag');
      if (!logger) return noop;
      const logFn = logger[funcName];
      return logFn;

but it'd mean if you mutate globalThis later to add log functions, they wouldn't be picked up. so maybe not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants