Skip to content
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

model: make ID fields non-pointer string types #3853

Merged
merged 4 commits into from
Jun 8, 2020

Conversation

axw
Copy link
Member

@axw axw commented Jun 8, 2020

Motivation/summary

Make more ID fields non-pointer types.

I missed a subtle issue with #3849, which changed some code to store a pointer to an empty parent ID in some cases. This didn't cause any change in the fields output, because we check != nil and != "" when producing beat events. However, when aggregating transaction metrics, we are only checking != nil.

To simplify the logic, and avoid these kinds of errors in the future, just store a string value. As a side benefit, this reduces heap allocations.

$ benchstat /tmp/old.txt /tmp/new.txt
name                                          old time/op    new time/op    delta                                                                                                                          
BackendProcessor/heavy.ndjson/NoRateLimit-12    38.4ms ± 4%    33.2ms ± 7%  -13.51%  (p=0.008 n=5+5)                                                                                                       
                                                                                                                                                                                                           
name                                          old speed      new speed      delta                                                                                                                          
BackendProcessor/heavy.ndjson/NoRateLimit-12  10.4MB/s ± 4%  12.0MB/s ± 7%  +15.69%  (p=0.008 n=5+5)                                                                                                       
                                                                                                                                                                                                           
name                                          old alloc/op   new alloc/op   delta                                                                                                                          
BackendProcessor/heavy.ndjson/NoRateLimit-12    6.34MB ± 0%    6.33MB ± 0%     ~     (p=0.151 n=5+5)
                                                                                                     
name                                          old allocs/op  new allocs/op  delta
BackendProcessor/heavy.ndjson/NoRateLimit-12      113k ± 0%      112k ± 0%   -1.27%  (p=0.008 n=5+5)

Included is a tangentially related change to stop recording the parent span index, used for RUMv3, on the model types. We now just use the index during decoding and then discard it.

Checklist

I have considered changes for:
- [ ] documentation
- [ ] logging (add log lines, choose appropriate log selector, etc.)
- [ ] metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. Kibana#44001)

How to test these changes

make test

Related issues

None.

@axw axw added the v7.9.0 label Jun 8, 2020
@apmmachine
Copy link
Contributor

apmmachine commented Jun 8, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #3853 updated]

  • Start Time: 2020-06-08T06:42:36.850+0000

  • Duration: 46 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 3204
Skipped 146
Total 3350

@axw axw merged commit 9a1bcfb into elastic:master Jun 8, 2020
@axw axw deleted the model-nonpointer-ids branch June 8, 2020 07:31
axw added a commit to axw/apm-server that referenced this pull request Jun 8, 2020
* model: make IDs non-pointer fields
* model: stop recording ParentIdx in model types
axw added a commit that referenced this pull request Jun 8, 2020
* model: make IDs non-pointer fields
* model: stop recording ParentIdx in model types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants