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.

Sign up for GitHub

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

Jump to bottom

[all] Replace multierr by errors.Join #8630

Closed
wants to merge 4 commits into from
Closed

[all] Replace multierr by errors.Join #8630

wants to merge 4 commits into from

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Oct 6, 2023 •
edited
Loading

Description:

Replaces go.uber.org/multierr by errors.Join.

Link to tracking Issue: Fixes #8210

@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 6, 2023
@mx-psi mx-psi force-pushed the mx-psi/multierr branch 3 times, most recently from 6ac024b to 61ab3d0 Compare October 6, 2023 11:16
@mx-psi mx-psi marked this pull request as ready for review October 6, 2023 11:25
@mx-psi mx-psi requested review from a team and djaglowski October 6, 2023 11:25
@codecov
Copy link

codecov bot commented Oct 6, 2023 •
edited
Loading

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Files Coverage Δ
cmd/builder/internal/builder/config.go 68.67% <100.00%> (ø)
component/componenttest/configtest.go 90.90% <100.00%> (ø)
component/config.go 87.50% <100.00%> (ø)
confmap/resolver.go 96.87% <100.00%> (ø)
exporter/exporterhelper/obsexporter.go 98.02% <100.00%> (+0.01%) ⬆️
featuregate/flag.go 100.00% <100.00%> (ø)
internal/fanoutconsumer/logs.go 100.00% <100.00%> (ø)
internal/fanoutconsumer/metrics.go 100.00% <100.00%> (ø)
internal/fanoutconsumer/traces.go 100.00% <100.00%> (ø)
obsreport/obsreporttest/obsreporttest.go 86.36% <100.00%> (ø)
... and 17 more

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@mx-psi
Copy link
Member Author

mx-psi commented Oct 6, 2023

@dmitryax I had to re-run the tests on this PR a few times because of a data race in exporterhelper which I believe is unrelated to this PR and instead related to #8625 since the PR touches the test that was failing (TestQueuedRetryPersistentEnabled_shutdown_dataIsRequeued):

Data race in tests

https://github.com/open-telemetry/opentelemetry-collector/actions/runs/6431268234/job/17465127695?pr=8630

==================
WARNING: DATA RACE
Write at 0x00c0000fce50 by goroutine 274:
  runtime.closechan()
      /opt/hostedtoolcache/go/1.21.1/x64/src/runtime/chan.go:357 +0x0
  go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*boundedMemoryQueue).Stop()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/internal/bounded_memory_queue.go:88 +0x64
  go.opentelemetry.io/collector/exporter/exporterhelper.(*producerConsumerQueueWithCounter).Stop()
      <autogenerated>:1 +0x43
  go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).shutdown()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender.go:157 +0xe4
  go.opentelemetry.io/collector/exporter/exporterhelper.(*baseExporter).Shutdown()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/common.go:221 +0x8a
  go.opentelemetry.io/collector/exporter/exporterhelper.TestQueuedRetryPersistentEnabled_shutdown_dataIsRequeued()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender_test.go:330 +0x6ea
  testing.tRunner()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1648 +0x44

Previous read at 0x00c0000fce50 by goroutine 275:
  runtime.chansend()
      /opt/hostedtoolcache/go/1.21.1/x64/src/runtime/chan.go:160 +0x0
  go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*boundedMemoryQueue).Produce()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/internal/bounded_memory_queue.go:75 +0xf3
  go.opentelemetry.io/collector/exporter/exporterhelper.(*producerConsumerQueueWithCounter).Produce()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/retry_sender_test.go:405 +0x79
  go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).onTemporaryFailure()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender.go:97 +0x357
  go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).onTemporaryFailure-fm()
      <autogenerated>:1 +0x73
  go.opentelemetry.io/collector/exporter/exporterhelper.(*retrySender).send()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/retry_sender.go:189 +0x1182
  go.opentelemetry.io/collector/exporter/exporterhelper.(*baseRequestSender).send()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/common.go:36 +0x51
  go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).start.func1()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender.go:122 +0x55
  go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*boundedMemoryQueue).Start.func1()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/internal/bounded_memory_queue.go:52 +0x156

Goroutine 274 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1648 +0x82a
  testing.runTests.func1()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1595 +0x238
  testing.runTests()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:2052 +0x896
  testing.(*M).Run()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1925 +0xb57
  main.main()
      _testmain.go:261 +0x2bd

Goroutine 275 (running) created at:
  go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*boundedMemoryQueue).Start()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/internal/bounded_memory_queue.go:47 +0xcd
  go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).start()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender.go:118 +0x238
  go.opentelemetry.io/collector/exporter/exporterhelper.(*baseExporter).Start()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/common.go:213 +0x1d7
  go.opentelemetry.io/collector/exporter/exporterhelper.TestQueuedRetryPersistentEnabled_shutdown_dataIsRequeued()
      /home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender_test.go:312 +0x357
  testing.tRunner()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.21.1/x64/src/testing/testing.go:1648 +0x44
==================
--- FAIL: TestQueuedRetryPersistentEnabled_shutdown_dataIsRequeued (1.02s)
    queue_sender_test.go:331: 
        	Error Trace:	/home/runner/work/opentelemetry-collector/opentelemetry-collector/exporter/exporterhelper/queue_sender_test.go:331
        	Error:      	Condition never satisfied
        	Test:       	TestQueuedRetryPersistentEnabled_shutdown_dataIsRequeued
    testing.go:[146](https://github.com/open-telemetry/opentelemetry-collector/actions/runs/6431268234/job/17465127695?pr=8630#step:6:147)5: race detected during execution of test
FAIL
FAIL	go.opentelemetry.io/collector/exporter/exporterhelper	1.407s

Could you take a look and confirm that this PR is unrelated to this flaky test?

bogdandrutu
bogdandrutu reviewed Oct 6, 2023
View reviewed changes
component/componenttest/configtest.go
Comment on lines +39 to +49
var errs []error

switch t.Kind() {
case reflect.Ptr:
errs = multierr.Append(errs, validateConfigDataType(t.Elem()))
errs = append(errs, validateConfigDataType(t.Elem()))
case reflect.Struct:
// Reflect on the pointed data and check each of its fields.
nf := t.NumField()
for i := 0; i < nf; i++ {
f := t.Field(i)
errs = multierr.Append(errs, checkStructFieldTags(f))
errs = append(errs, checkStructFieldTags(f))
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't matter that much here, but this adds lots of allocations. Do we care? Do we want to have a helper type (internal) "MultiError" with an append that does not append to the array if nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I don't think we care in most places but there are some where we definitely do care; e.g. the fanoutconsumer Consume* calls seems like a place where we should avoid this.

Should we use the helper just for those places? We could also just continue using multierr on those for now and report this on golang/go

Copy link
Member

Choose a reason for hiding this comment

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

report this on golang/go

I think we should definitely do that.

Should we use the helper just for those places? We could also just continue using multierr on those for now and

I think we can continue to use multierr for that.

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added Stale and removed Stale labels Oct 31, 2023
@github-actions GitHub Actions
Copy link
Contributor

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 Nov 21, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023

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

@github-actions github-actions bot closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogdandrutu bogdandrutu bogdandrutu left review comments

@djaglowski djaglowski djaglowski approved these changes

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Go 1.20 errors.Join instead of go.uber.org/multierr
3 participants
@mx-psi @bogdandrutu @djaglowski