Skip to content

--do not merge-- Benchmark Branch for storage key-based Request #5

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

Conversation

jackgopack4
Copy link

Description

run benchmark for Persistent queue with following approach:

  • store SpanContext as a separate key and make separate Storage operations for Request and Context

Link to tracking issue

Relates to open-telemetry#12934

Testing

Runs benchmark with persistent queue with filebacked storage extension

mx-psi and others added 14 commits May 8, 2025 22:53
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This updates the bugfix guidelines to make them less strict. In
particular, after this change, we would start releasing bugfix releases
under the following cases:

1. Lack of consensus of SIG leads on whether to release a bugfix version
within one working day after a report has been made
2. Issues that have not been reported by multiple people, but that are
known to be used in production

This also:
- Explicitly lists difficulties with debugging and troubleshooting as
'severe enough'
- Explicitly states that the release manager is responsible for bugfix
releases
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

While working on open-telemetry#12981, I would have found it useful to have these
tests, since they surfaced a bug when enabling `DecodeNil`.
…etry#13001)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

If we enable `DecodeNil` as true, we may have [untyped
nils](https://go.dev/doc/faq#nil_error) being passed. Unfortunately,
these are not valid values for `reflect`, which leads to surprising
behavior such as golang/go/issues/51649.

Unfortunately, the default hooks from mapstructure do not deal with this
properly. To account for this, we:
- Vendor and change the `ComposeDecodeHookFunc` function so that this
case is accounted for the kinds of hooks that just won't work with
untyped nils
- Create a safe wrapper for the hooks that do work with untyped nils.
This wrapper is used in all hooks, but in the interest of keeping as
close to what I would imagine upstream will accept, I did not add this
to the compose function.

This should not have any end-user observable behavior.

<!-- Issue number if applicable -->
#### Link to tracking issue

Attempt to work around
open-telemetry#12996 (comment)

---------

Co-authored-by: Evan Bradley <[email protected]>
@jackgopack4
Copy link
Author

go test -benchmem -run=^$ -bench ^BenchmarkPersistentQueue_PtraceTraces$ go.opentelemetry.io/collector/exporter/exporterhelper/internal/queuebatch

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/collector/exporter/exporterhelper/internal/queuebatch
cpu: Apple M3 Max
BenchmarkPersistentQueue_PtraceTraces-16    	    3501	    296079 ns/op	 4146273 B/op	     650 allocs/op
PASS
ok  	go.opentelemetry.io/collector/exporter/exporterhelper/internal/queuebatch	2.269s

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2025

Codecov Report

Attention: Patch coverage is 79.48718% with 40 lines in your changes missing coverage. Please review.

Project coverage is 91.58%. Comparing base (ac520a5) to head (5c95f23).
Report is 111 commits behind head on main.

Files with missing lines Patch % Lines
...rterhelper/internal/queuebatch/persistent_queue.go 81.66% 15 Missing and 7 partials ⚠️
...p/internal/third_party/composehook/compose_hook.go 76.19% 14 Missing and 1 partial ⚠️
confmap/confmap.go 75.00% 2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (79.48%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
- Coverage   91.67%   91.58%   -0.10%     
==========================================
  Files         503      504       +1     
  Lines       27819    27981     +162     
==========================================
+ Hits        25502    25625     +123     
- Misses       1828     1859      +31     
- Partials      489      497       +8     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 24, 2025
Copy link

github-actions bot commented Jun 7, 2025

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jun 7, 2025
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.

4 participants