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

Move global random number generator to randRes field #5819

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Sep 13, 2024

Instead of using a global random number generator for all randRes, have each value use its own. This removes the need for locking and managing concurrent safe access to the global. Also, the field, given the Reservoir type is not concurrent safe and the metric pipeline guards this, does not need a sync.Mutex to guard it.

Supersedes #5815
Fix #5814

Performance Analysis

This change has approximately equivalent performance as the existing code based on existing benchmarks.

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                           │   old.txt   │              new.txt               │
                           │   sec/op    │   sec/op     vs base               │
Exemplars/Int64Counter/8-8   14.00µ ± 3%   13.44µ ± 4%  -3.98% (p=0.001 n=10)

                           │   old.txt    │             new.txt              │
                           │     B/op     │     B/op      vs base            │
Exemplars/Int64Counter/8-8   3.791Ki ± 0%   3.791Ki ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

                           │  old.txt   │            new.txt             │
                           │ allocs/op  │ allocs/op   vs base            │
Exemplars/Int64Counter/8-8   84.00 ± 0%   84.00 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (a7e83aa) to head (9f4e8cd).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5819   +/-   ##
=====================================
  Coverage   84.5%   84.5%           
=====================================
  Files        272     272           
  Lines      22756   22758    +2     
=====================================
+ Hits       19248   19252    +4     
+ Misses      3165    3163    -2     
  Partials     343     343           

see 2 files with indirect coverage changes

@MrAlias

This comment was marked as outdated.

@MrAlias

This comment was marked as outdated.

Instead of using a global random number generator for all `randRes`,
have each value use its own. This removes the need for locking and
managing concurrent safe access to the global. Also, the field, given
the `Reservoir` type is not concurrent safe and the metric pipeline
guards this, does not need a `sync.Mutex` to guard it.
@MrAlias MrAlias merged commit 42fd8fe into open-telemetry:main Sep 16, 2024
32 checks passed
@MrAlias MrAlias deleted the exemplar-no-global-rng branch September 16, 2024 14:31
@MrAlias MrAlias added this to the v1.31.0 milestone Oct 10, 2024
dashpole added a commit that referenced this pull request Oct 11, 2024
### Added

- Add `go.opentelemetry.io/otel/sdk/metric/exemplar` package which
includes `Exemplar`, `Filter`, `TraceBasedFilter`, `AlwaysOnFilter`,
`HistogramReservoir`, `FixedSizeReservoir`, `Reservoir`, `Value` and
`ValueType` types. These will be used for configuring the exemplar
reservoir for the metrics sdk. (#5747, #5862)
- Add `WithExportBufferSize` option to log batch processor.(#5877)

### Changed

- Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`.
Exemplars can be disabled by setting
`OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778)
- `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly
introduced `EnabledParameters` type instead of `Record`. (#5791)
- `FilterProcessor.Enabled` in
`go.opentelemetry.io/otel/sdk/log/internal/x` now accepts
`EnabledParameters` instead of `Record`. (#5791)
- The `Record` type in `go.opentelemetry.io/otel/log` is no longer
comparable. (#5847)
- Performance improvements for the trace SDK `SetAttributes` method in
`Span`. (#5864)
- Reduce memory allocations for the `Event` and `Link` lists in `Span`.
(#5858)
- Performance improvements for the trace SDK `AddEvent`, `AddLink`,
`RecordError` and `End` methods in `Span`. (#5874)

### Deprecated

- Deprecate all examples under `go.opentelemetry.io/otel/example` as
they are moved to [Contrib
repository](https:/open-telemetry/opentelemetry-go-contrib/tree/main/examples).
(#5854)

### Fixed

- The race condition for multiple `FixedSize` exemplar reservoirs
identified in #5814 is resolved. (#5819)
- Fix log records duplication in case of heterogeneous resource
attributes by correctly mapping each log record to it's resource and
scope. (#5803)
- Fix timer channel drain to avoid hanging on Go 1.23. (#5868)
- Fix delegation for global meter providers, and panic when calling
otel.SetMeterProvider. (#5827)
- Change the `reflect.TypeOf` to use a nil pointer to not allocate on
the heap unless necessary. (#5827)
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.

Missing lock around rng access in rand.go
4 participants