transaction: Stop recording breakdown metrics#1167
transaction: Stop recording breakdown metrics#1167marclop merged 4 commits intoelastic:masterfrom marclop:f/stop-recording-transaction-breakdown-metrics
Conversation
Stops recording the unused transaction breakdown metrics:
* `transaction.duration.{count,sum.us}`
* `transaction.breakdown.count`
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
| ) | ||
| require.NotNil(t, breakdownTimingObj) | ||
| assert.Equal(t, []int{0}, breakdownTimingFieldIndex) | ||
| assert.Equal(t, []int{1}, breakdownTimingFieldIndex) |
There was a problem hiding this comment.
Had to change the index of the field for the test to pass since I've reordered them.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
breakdown.go
Outdated
| // have calculated breakdown metrics. If breakdown metrics are | ||
| // enabled, this will be equal transaction.count. | ||
| breakdownCount uintptr | ||
| // Padding to ensure the span field below is 64-bit aligned. |
There was a problem hiding this comment.
Comment needs updating. Do we still need the padding? IIRC we want the struct size to be a multiple of 8 bytes?
There was a problem hiding this comment.
I tried removing the padding, but on 32-bit architecture the uintptr is 4 bytes rather than 8 (64-bit), making the total struct size 12 bytes in 32-bit architectures instead of the 16 that we get by default on 32-bit architectures.
We could arguably change the implementation to always use an uint64, which gives us a maximum value of 18,446,744,073,709,551,615 which is quite large already, not sure what benefit uintptr would give us since we're not using it for pointer references anyway.
I think switching to uint64 makes sense since instead of allocating padding bytes in memory, we'd use them for the actual count rather than having padding on 32-bit architectures, I'll make the change, but re-request a review from you to make sure we're ok with uint64 vs uintptr.
There was a problem hiding this comment.
Looks sensible, and simpler - thanks.
Removes the 4 bytes of padding in `breakdownTiming` and changes the type of `spanTiming.count` from `uintptr` to `uint64`, making 32 and 64 bit architectures span counts equal in size, removing the need for padding. Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
|
I don't think this can usefully be tested manually, but I'm open to ideas. The server doesn't record these metrics any more. |
3 participants
Description
Stops recording the unused transaction breakdown metrics:
transaction.duration.{count,sum.us}transaction.breakdown.countRemoves the 4 bytes of padding in
breakdownTimingand changes the typeof
spanTiming.countfromuintptrtouint64, making 32 and 64 bitarchitectures span counts equal in size, removing the need for padding.
Related issues
Closes #1139