-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[exporterhelper] Remove redundant request interfaces #8867
[exporterhelper] Remove redundant request interfaces #8867
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8867 +/- ##
==========================================
+ Coverage 90.87% 90.90% +0.02%
==========================================
Files 317 317
Lines 17243 17229 -14
==========================================
- Hits 15670 15662 -8
+ Misses 1283 1277 -6
Partials 290 290 ☔ View full report in Codecov by Sentry. |
163a21a
to
8ac78a7
Compare
8ac78a7
to
0f52e10
Compare
// Request represents a single request that can be sent to an external endpoint. | ||
// This API is at the early stage of development and may change without backward compatibility | ||
// until https://github.com/open-telemetry/opentelemetry-collector/issues/8122 is resolved. | ||
type Request interface { |
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.
For the moment can we keep this internal, since the usage is still internal anyway.
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.
Maybe keep it in internal for the moment.
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.
The usage is not only internal. This is a public interface used by the new experimental exporterhelper. We are moving to another place
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.
Let't do that in a separate PR then?
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 don't think we need it in a separate package, that's why I don't want to have it.
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 think we already had everywhere using the internal.Request
, and I am confused why we had to change too many things?
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.
What I'm suggesting in this PR is to stop using the request
and internal.Request
and use the original Request
in the senders along with the context. I think this change significantly simplifies code. The only place where we need to wrap it is the queue/internal package.
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.
What I am saying is that I don't believe long term we want request in a separate package, so better not to put it there 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.
Keep in mind that go is duck typing, which means for the moment you can define the interface twice one in exporter and one in internal and everything works.
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.
Ok, I moved the Request back to the exporterhelper package
|
||
// RequestMarshaler defines a function which takes a request and marshals it into a byte slice | ||
type RequestMarshaler func(Request) ([]byte, error) | ||
func (qr *QueueRequest) Context() context.Context { |
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.
Why do you need this if you can access the internal member?
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 being used by the queue sender for sending the context to the next sender
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.
Why not passing it in the callback?
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 tried that. In that case, the QueueRequest is left with onProcessingFinishedFunc
func only. So we can pass them as three fields, but the callback function gets pretty complicated.
Callback func(ctx context.Context, req request.Request, onProcessingFinishedFunc func())
Not sure if keeping the onProcessingFinishedFunc field only warrants having the struct like
type QueueRequest struct {
request.Request
onProcessingFinishedFunc func()
}
Let me know WDYT
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.
Maybe keep onProcessingFinishedFunc
as a prop on the Request and wrap the original request in case of the PersistenQueue?
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 essentially the same as what's implemented in this PR, just with removed Context field, right?
type QueueRequest struct {
request.Request
onProcessingFinishedFunc func()
}
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.
Yes and no. Also change https://github.com/open-telemetry/opentelemetry-collector/pull/8867/files#diff-667c34fb5331f087f510bdf2b6f39e626e6a9e6d9003425b26236b660e3a5be2R17 to be Callback func(context.Context, Request)
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 realized that we need the context to be embedded for the channel interface we want to have in #8828
// Request represents a single request that can be sent to an external endpoint. | ||
// This API is at the early stage of development and may change without backward compatibility | ||
// until https://github.com/open-telemetry/opentelemetry-collector/issues/8122 is resolved. | ||
type Request interface { |
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.
Maybe keep it in internal for the moment.
// ItemsCount returns a number of basic items in the request where item is the smallest piece of data that can be | ||
// sent. For example, for OTLP exporter, this value represents the number of spans, | ||
// metric data points or log records. | ||
ItemsCount() int |
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.
One thing that you need to make sure, especially for batching is that it is very bad for some systems to split the metric across requests. Even if the limit may be at data points, you may want to create an issue for your batcher work.
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.
Makes sense. We can probably add comments to the batcher config recommending users to keep the difference between MinSizeItems
and MaxSizeItems
high and implement batching for OTLP exporter to make the best effort not to split metrics with those restrictions. WDYT?
Pass request+context through the senders pipeline similar to what we do in the collector's pipeline. Use a helper QueueRequest struct for passing requests through queues
0f52e10
to
19b766f
Compare
item.OnProcessingFinished() | ||
Callback: func(qr internal.QueueRequest) { | ||
_ = qs.nextSender.send(qr.Context, qr.Request.(Request)) | ||
// TODO: Update OnProcessingFinished to accept error and remove the retry->queue sender callback. |
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.
Nice :)
Follow-up to #8867. Forgot to update the changelog entry after addressing the PR review comments
Pass request+context through the senders pipeline similar to what we do in the collector's pipeline. Use a helper QueueRequest struct for passing requests through queues
This changes also moves the experimental Request interface to a separate package.