-
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
[exporter][batcher] Add the API ByteSize() to exporter.Request #12153
base: main
Are you sure you want to change the base?
[exporter][batcher] Add the API ByteSize() to exporter.Request #12153
Conversation
cf461ca
to
5d09bf6
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (66.66%) 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 #12153 +/- ##
==========================================
- Coverage 91.75% 91.21% -0.54%
==========================================
Files 464 466 +2
Lines 24763 25668 +905
==========================================
+ Hits 22722 23414 +692
- Misses 1656 1841 +185
- Partials 385 413 +28 ☔ View full report in Codecov by Sentry. |
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.
Could we add tests?
5d09bf6
to
bba043f
Compare
@dmathieu Hi Damien, I added a couple of tests to check the new API. Would you mind taking another look? |
exporter/exporterhelper/logs_test.go
Outdated
@@ -43,6 +43,8 @@ var ( | |||
|
|||
func TestLogsRequest(t *testing.T) { | |||
lr := newLogsRequest(testdata.GenerateLogs(1), nil) | |||
assert.Equal(t, 1, lr.ItemsCount()) | |||
assert.Equal(t, logsMarshaler.LogsSize(testdata.GenerateLogs(1)), lr.ByteSize()) |
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.
IMHO, this (and the other tests) should test against a static value.
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.
Sounds good!
Description
This PR adds the API to
ByteSize()
toexporter.Request
Link to tracking issue
#3262
Testing
Documentation