Send meta_struct properties as byte array #4349
Conversation
Overall package sizeSelf size: 6.53 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-05-27 13:44:54 Comparing candidate commit 3006ec0 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 261 metrics, 4 unstable metrics. scenario:plugin-graphql-with-depth-and-collapse-on-18
|
packages/dd-trace/src/encode/0.4.js
Outdated
| const keys = Array.isArray(value) ? [] : Object.keys(value) | ||
| const validKeys = keys.filter(key => | ||
| typeof value[key] === 'string' || | ||
| typeof value[key] === 'number' || | ||
| (value[key] !== null && typeof value[key] === 'object')) | ||
|
|
||
| this._encodeMapPrefix(bytes, validKeys.length) | ||
|
|
||
| for (const key of validKeys) { | ||
| this._encodeString(bytes, key) | ||
| this._encodeObjectAsByteArray(bytes, value[key]) | ||
| } |
There was a problem hiding this comment.
I'm vacillating on recommending using Object.entries here. On one hand, it creates more temporary small arrays. On the other hand, it eliminates the repetitive evaluation of value[key] and makes the filter predicate context-free, which I think are nice properties for reading comprehension of the code.
At the end of the day, it's a matter of preference, but thought I'll raise it for consideration.
| const keys = Array.isArray(value) ? [] : Object.keys(value) | |
| const validKeys = keys.filter(key => | |
| typeof value[key] === 'string' || | |
| typeof value[key] === 'number' || | |
| (value[key] !== null && typeof value[key] === 'object')) | |
| this._encodeMapPrefix(bytes, validKeys.length) | |
| for (const key of validKeys) { | |
| this._encodeString(bytes, key) | |
| this._encodeObjectAsByteArray(bytes, value[key]) | |
| } | |
| const entries = Array.isArray(value) ? [] : Object.entries(value) | |
| const validEntries = entries.filter(([, v]) => | |
| typeof v === 'string' || | |
| typeof v === 'number' || | |
| (v !== null && typeof v === 'object')) | |
| this._encodeMapPrefix(bytes, validEntries.length) | |
| for (const [k, v] of validEntries) { | |
| this._encodeString(bytes, k) | |
| this._encodeObjectAsByteArray(bytes, v) | |
| } |
There was a problem hiding this comment.
Ok, I've done the changes. And changes also the other method that I made in the previous PR to use the same approach. thanks
There was a problem hiding this comment.
I came back to the previous approach. Even if Object.entries is easier to understand, it creates multiple arrays increasing memory and CPU usage.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4349 +/- ##
===========================================
- Coverage 85.94% 69.19% -16.76%
===========================================
Files 117 1 -116
Lines 4384 198 -4186
Branches 33 33
===========================================
- Hits 3768 137 -3631
+ Misses 616 61 -555 ☔ View full report in Codecov by Sentry. |
What does this PR do?
Sends meta_struct data as byte array instead of as object.
Motivation
It was expected to be a byte array and I misunderstood the spec it in the first implementation.
Plugin Checklist