-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Move receiver builder into internal service #10781
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10781 +/- ##
==========================================
+ Coverage 91.66% 91.68% +0.02%
==========================================
Files 405 406 +1
Lines 18979 19032 +53
==========================================
+ Hits 17397 17450 +53
Misses 1223 1223
Partials 359 359 ☔ View full report in Codecov by Sentry. |
2c8a143
to
0cc2628
Compare
0cc2628
to
f598cd0
Compare
96906e5
to
c610f18
Compare
service/service.go
Outdated
// | ||
// Deprecated: use the [ReceiversConfigs] and [ReceiversFactories] options | ||
// instead. | ||
Receivers builders.Receiver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change since it's a different type now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we could make it not a breaking change if we make builders.Receiver
an alias to receiver.Builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that a breaking change?
We're indeed changing it from a struct to an interface, but they match each other, so passing the struct will still work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is not a breaking change, @dmitryax could you show an example where this would break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine if someone extracts the field value and expects a specific type there. That code will break. But that's fine I think. I missed that it's an interface matching the old struct.
7a903af
to
5892a98
Compare
service/service.go
Outdated
// | ||
// Deprecated: use the [ReceiversConfigs] and [ReceiversFactories] options | ||
// instead. | ||
Receivers builders.Receiver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is not a breaking change, @dmitryax could you show an example where this would break?
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description There builders were deprecated in 0.108.0, so they can be removed now. PRs that deprecated the builders: * #10781 * #10782 * #10783 * #10784 * #10785 cc @mx-psi
Description
This moves the receiver builder out of the
receiver
package, and intoservice/internal/builders
.There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it.
Link to tracking issue
#10375 (review)