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

Make the collector exporter mutate data, and remove unnecessary CopyTo #892

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Sep 12, 2024

Looking at #890 (comment)

This change makes the collector exporter now mutate data. We were doing multiple layers additional copying in our exporter to prevent mutating data, but it is likely better for us to just be a mutating exporter, and let the framework handle it.

This keeps the collector code clean, and should have better CPU/Memory usage (even after the additional copy) since we are copying fewer times.

Benchmarks (including those added in #893):

goos: linux
goarch: amd64
pkg: github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/collector/internal/normalization
cpu: AMD EPYC 7B12
                                              │   main.txt    │             mutating.txt              │
                                              │    sec/op     │    sec/op     vs base                 │
NormalizeNumberDataPoint-2                      1845.0n ± ∞ ¹   532.6n ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeHistogramDataPoint-2                   2314.0n ± ∞ ¹   615.7n ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeExopnentialHistogramDataPoint-2        2071.0n ± ∞ ¹   673.7n ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeSummaryDataPoint-2                     1718.0n ± ∞ ¹   465.9n ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeNumberDataPoint-2                 2094.0n ± ∞ ¹   826.3n ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeHistogramDataPoint-2               2.565µ ± ∞ ¹   1.043µ ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeExponentialHistogramDataPoint-2   2148.0n ± ∞ ¹   869.4n ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeSummaryDataPoint-2                1775.0n ± ∞ ¹   836.2n ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                                          2.049µ         709.9n        -65.36%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                                              │   main.txt   │             mutating.txt             │
                                              │     B/op     │    B/op      vs base                 │
NormalizeNumberDataPoint-2                      408.00 ± ∞ ¹   16.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeHistogramDataPoint-2                   536.00 ± ∞ ¹   32.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeExopnentialHistogramDataPoint-2        600.00 ± ∞ ¹   48.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeSummaryDataPoint-2                     320.00 ± ∞ ¹   16.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeNumberDataPoint-2                  504.0 ± ∞ ¹   128.0 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeHistogramDataPoint-2               745.0 ± ∞ ¹   256.0 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeExponentialHistogramDataPoint-2    792.0 ± ∞ ¹   256.0 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeSummaryDataPoint-2                 432.0 ± ∞ ¹   128.0 ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                                          520.6         67.33        -87.07%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                                              │   main.txt   │             mutating.txt             │
                                              │  allocs/op   │  allocs/op   vs base                 │
NormalizeNumberDataPoint-2                      10.000 ± ∞ ¹   2.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeHistogramDataPoint-2                   13.000 ± ∞ ¹   3.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeExopnentialHistogramDataPoint-2        14.000 ± ∞ ¹   4.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
NormalizeSummaryDataPoint-2                      8.000 ± ∞ ¹   1.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeNumberDataPoint-2                 12.000 ± ∞ ¹   4.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeHistogramDataPoint-2              17.000 ± ∞ ¹   7.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeExponentialHistogramDataPoint-2   14.000 ± ∞ ¹   4.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
ResetNormalizeSummaryDataPoint-2                11.000 ± ∞ ¹   4.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                                          12.09         3.191        -73.61%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

@dashpole dashpole requested a review from a team as a code owner September 12, 2024 16:19
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 54.44444% with 41 lines in your changes missing coverage. Please review.

Project coverage is 63.03%. Comparing base (4caace7) to head (c748e16).
Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
...ctor/internal/normalization/standard_normalizer.go 61.11% 28 Missing ⚠️
...ctor/internal/normalization/disabled_normalizer.go 0.00% 12 Missing ⚠️
exporter/collector/metrics.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #892      +/-   ##
==========================================
+ Coverage   61.03%   63.03%   +1.99%     
==========================================
  Files          56       57       +1     
  Lines        5903     5967      +64     
==========================================
+ Hits         3603     3761     +158     
+ Misses       2143     2044      -99     
- Partials      157      162       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dashpole
Copy link
Contributor Author

dashpole commented Sep 12, 2024

depends on #893

@aabmass
Copy link
Contributor

aabmass commented Sep 12, 2024

It's weird the benchmarks now have ± ∞ ¹ vs #890 which had meaningful bounds. Any idea what's going on?

@dashpole
Copy link
Contributor Author

I only did -count=1 because I was running short on time. I can re-run if you want

Copy link
Contributor

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Where is the actual flag indicating that the exporter is mutating? I'm guessing collector-contrib in a separate PR

@dashpole
Copy link
Contributor Author

It is an option to exporterhelper: https://pkg.go.dev/go.opentelemetry.io/collector/exporter/exporterhelper#WithCapabilities, which we will need to set in our exporters

@dashpole dashpole merged commit ac83b74 into GoogleCloudPlatform:main Sep 19, 2024
33 of 34 checks passed
@dashpole dashpole deleted the mutating branch September 19, 2024 16:13
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