-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
awss3exporter: Add Protocol Buf storage format #30682
awss3exporter: Add Protocol Buf storage format #30682
Conversation
m, err := newMarshaler("otlp_proto", zap.NewNop()) | ||
assert.NoError(t, err) | ||
require.NotNil(t, m) | ||
assert.Equal(t, m.format(), "binpb") |
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.
Is binpb
a good term here?
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 did have to look up what a binary protocol buffer file extension should be, according to the Protocol Buf site:
https://protobuf.dev/programming-guides/techniques/#suffixes
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, thanks for the link! :)
Co-authored-by: Antoine Toulme <[email protected]>
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.
Looks tight, LGTM
Any @open-telemetry/collector-contrib-maintainer willing to give their blessing and merge this PR? |
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 dont see any tests that prove these changes do what they claim to do, were you able to test this locally an confirm the proto marshalling works as expected?
I was indeed able to confirm that the existing Protobuf Marshaler does generate a byte stream that can be unmarshaled when read back from S3. |
**Description:** Add ProtoBuf output format to the AWS S3 exporter **Link to tracking Issue:** open-telemetry#30681 **Testing:** Additional unit tests have been added for the new format type and I've also exported data to an S3 bucket in Protobuf format and read it back to confirm that it's working end to end. **Documentation:** Added details of the new marshaler format to the README.md --------- Co-authored-by: Antoine Toulme <[email protected]>
Description: Add ProtoBuf output format to the AWS S3 exporter
Link to tracking Issue: #30681
Testing: Additional unit tests have been added for the new format type and I've also exported data to an S3 bucket in Protobuf format and read it back to confirm that it's working end to end.
Documentation: Added details of the new marshaler format to the README.md