[exporter] moved mergeBatchFunc and mergeBatchSplitFunc to request #11459
Conversation
sfc-gh-sili
force-pushed
the
sili-merge
branch
2 times, most recently
from
287ad0e
to
39122c8
Compare
sfc-gh-sili
force-pushed
the
sili-merge
branch
from
39122c8
to
6f5643a
Compare
sfc-gh-sili
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11459 +/- ##
==========================================
- Coverage 92.14% 91.42% -0.72%
==========================================
Files 433 433
Lines 20389 23658 +3269
==========================================
+ Hits 18787 21629 +2842
- Misses 1238 1655 +417
- Partials 364 374 +10 ☔ View full report in Codecov by Sentry. |
exporter/internal/request.go
Outdated
| Merge(context.Context, BatchRequest) (BatchRequest, error) | ||
| MergeSplit(context.Context, exporterbatcher.MaxSizeConfig, BatchRequest) ([]BatchRequest, error) |
There was a problem hiding this comment.
Please document the functions.
exporter/internal/request.go
Outdated
| Merge(context.Context, BatchRequest) (BatchRequest, error) | ||
| MergeSplit(context.Context, exporterbatcher.MaxSizeConfig, BatchRequest) ([]BatchRequest, error) |
There was a problem hiding this comment.
I think (maybe next PR) we can make the second func just Split.
There was a problem hiding this comment.
I tried this some time ago. AFAIR there was a performance hit coming from the Merge->Split sequence of calls. But I'd be happy if we can make it work with no problems
| func (req *tracesRequest) Merge(_ context.Context, _ exporterhelper.BatchRequest) (exporterhelper.BatchRequest, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // MergeSplit splits and/or merges the profiles into multiple requests based on the MaxSizeConfig. | ||
| func (req *tracesRequest) MergeSplit(_ context.Context, _ exporterbatcher.MaxSizeConfig, _ exporterhelper.BatchRequest) ( | ||
| []exporterhelper.BatchRequest, error) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Is there a test where tracesRequest needs the BatchRequest implementation?
There was a problem hiding this comment.
I agree this is confusing. tracesRequest in this file is solely for testing that merging profilesBatchRequest and tracesRequest would fail. Let me rename it to dummyRequest.
| } | ||
| return bs.sendMergeBatch(ctx, req) | ||
| return bs.sendMergeBatch(ctx, req.(internal.BatchRequest)) |
There was a problem hiding this comment.
Custom Request not implementing batching function would panic here. Let's return an error instead?
There was a problem hiding this comment.
It is ok for the moment to panic I believe since we are experimental. We will make it required to implement this to setup batching soon.
There was a problem hiding this comment.
@sfc-gh-sili for the moment move the functions on Request.
There was a problem hiding this comment.
@bogdandrutu Thanks!
Do you mean instead of having Request and BatchRequest, we would like to have Merge() and MergeSplitBatch() as a required function and then just have Request? For users who do not do batching they can leave Merge() and MergeSplit() as noop I assume?
There was a problem hiding this comment.
For users who do not do batching they can leave Merge() and MergeSplit() as noop I assume?
Correct.
sfc-gh-sili
force-pushed
the
sili-merge
branch
from
075207b
to
52d6e24
Compare
Description
This PR changes
mergeBatchFuncandmergeBatchSplitfunction as a member function ofbatchRequest.Link to tracking issue
Testing
Documentation