Skip to content
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

Merged

Conversation

adcharre
Copy link
Contributor

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

m, err := newMarshaler("otlp_proto", zap.NewNop())
assert.NoError(t, err)
require.NotNil(t, m)
assert.Equal(t, m.format(), "binpb")
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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! :)

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks tight, LGTM

@atoulme
Copy link
Contributor

atoulme commented Jan 29, 2024

Any @open-telemetry/collector-contrib-maintainer willing to give their blessing and merge this PR?

Copy link
Member

@TylerHelmuth TylerHelmuth left a 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?

@adcharre
Copy link
Contributor Author

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.

@TylerHelmuth TylerHelmuth merged commit a76c319 into open-telemetry:main Jan 29, 2024
85 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 29, 2024
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
**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 <antoine@toulme.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants