Skip to content

Conversation

sslotnick-isp
Copy link
Contributor

@sslotnick-isp sslotnick-isp commented May 29, 2020

This optimizes how tags are handled to minimize map to slice conversions. One of the nice things about this library is that tags can be specified as a map instead of having to munge tag keys and values together as is necessary with datadog-go. However, the current version converts between map and list on every metric call which leads to a lot of extra allocations. The main reason for maintaining the map at all was to ensure duplicate tags got overwritten. However, I don't know of any use cases where this kind of override is currently being used. Secondly, I tested how DataDog would handle duplicate tag keys and found that it will report both:
multiple-tags

Note: It shows the values comma separated but if I were to filter for just testapp:abc I would get both the yellow and purple lines.

The savings are significant in memory and CPU time:

benchmark                                       old ns/op     new ns/op     delta
Benchmark_0Tags_100Emits-8                      15389         17235         +12.00%
BenchmarkTags_5Tags_100Emits-8                  178638        24261         -86.42%
BenchmarkTags_5Tags_100Emits_WithInline-8       311105        90284         -70.98%
BenchmarkTags_10Tags_1000Emits-8                2652560       349187        -86.84%
BenchmarkTags_10Tags_1000Emits_WithInline-8     3836957       1051966       -72.58%
BenchmarkTags_15Tags_100Emits-8                 386667        47259         -87.78%
BenchmarkTags_15Tags_100Emits_WithInline-8      618854        121621        -80.35%

benchmark                                       old allocs     new allocs     delta
Benchmark_0Tags_100Emits-8                      2              1              -50.00%
BenchmarkTags_5Tags_100Emits-8                  1603           17             -98.94%
BenchmarkTags_5Tags_100Emits_WithInline-8       2403           717            -70.16%
BenchmarkTags_10Tags_1000Emits-8                31005          32             -99.90%
BenchmarkTags_10Tags_1000Emits_WithInline-8     39068          7033           -82.00%
BenchmarkTags_15Tags_100Emits-8                 4605           47             -98.98%
BenchmarkTags_15Tags_100Emits_WithInline-8      5408           747            -86.19%

benchmark                                       old bytes     new bytes     delta
Benchmark_0Tags_100Emits-8                      80            48            -40.00%
BenchmarkTags_5Tags_100Emits-8                  32369         368           -98.86%
BenchmarkTags_5Tags_100Emits_WithInline-8       107579        51905         -51.75%
BenchmarkTags_10Tags_1000Emits-8                641021        707           -99.89%
BenchmarkTags_10Tags_1000Emits_WithInline-8     1699559       596099        -64.93%
BenchmarkTags_15Tags_100Emits-8                 98174         1012          -98.97%
BenchmarkTags_15Tags_100Emits_WithInline-8      260658        68551         -73.70%

Note: This also adds back the dep files so that teams not on go mods will still be able to upgrade.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #14 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   93.80%   93.89%   +0.08%     
==========================================
  Files           6        6              
  Lines         420      426       +6     
==========================================
+ Hits          394      400       +6     
  Misses         18       18              
  Partials        8        8              
Impacted Files Coverage Δ
metrics/datadog.go 94.87% <100.00%> (-0.26%) ⬇️
metrics/logger.go 97.40% <100.00%> (ø)
metrics/recorder.go 97.18% <100.00%> (ø)
metrics/util.go 90.24% <100.00%> (+2.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af57a97...c02370b. Read the comment docs.

Copy link

@danielgtaylor-isp danielgtaylor-isp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Co-authored-by: Daniel Taylor <dtaylor@istreamplanet.com>
@sslotnick-isp sslotnick-isp merged commit daf7f42 into master Jun 10, 2020
@sslotnick-isp sslotnick-isp deleted the optimize-tags branch June 10, 2020 00:41
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.

3 participants