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

[receiver/awss3receiver] Initial implementation #32222

Merged
merged 35 commits into from
May 7, 2024

Conversation

adcharre
Copy link
Contributor

@adcharre adcharre commented Apr 8, 2024

Description: This is the initial implementation of the AWS S3 receiver. The receiver can load trace from an S3 bucket starting at the configured time until the stop time. Json and protobuf formats are supported along with gzip compression.

Link to tracking Issue: #30750

Testing: Unit tests added and read real trace from an S3 bucket.

Documentation: None added

@adcharre adcharre requested a review from atoulme as a code owner April 8, 2024 15:28
@adcharre adcharre requested a review from a team April 8, 2024 15:28
unmarshaler = &ptrace.JSONUnmarshaler{}
}
if strings.HasSuffix(key, ".binpb") {
unmarshaler = &ptrace.ProtoUnmarshaler{}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can maybe do something here in a future PR where the unmarshaler is dictated by config. I appreciate how this works well when you don't know what data to expect though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - at this stage just want to get the basics in and then add the ability to use extensions to unmarshall data next.

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

I didn't get a chance to look through tests yet, just an initial pass over functionality. I don't have much AWS experience, so some questions may be pretty elementary. 👍

receiver/awss3receiver/config.go Outdated Show resolved Hide resolved
receiver/awss3receiver/config.go Outdated Show resolved Hide resolved
receiver/awss3receiver/config.go Outdated Show resolved Hide resolved
receiver/awss3receiver/s3reader.go Outdated Show resolved Hide resolved
receiver/awss3receiver/s3reader.go Outdated Show resolved Hide resolved
receiver/awss3receiver/s3reader.go Outdated Show resolved Hide resolved
receiver/awss3receiver/s3reader.go Outdated Show resolved Hide resolved
receiver/awss3receiver/s3intf.go Outdated Show resolved Hide resolved
receiver/awss3receiver/receiver.go Outdated Show resolved Hide resolved
receiver/awss3receiver/s3reader.go Outdated Show resolved Hide resolved
@adcharre
Copy link
Contributor Author

adcharre commented Apr 9, 2024

I didn't get a chance to look through tests yet, just an initial pass over functionality. I don't have much AWS experience, so some questions may be pretty elementary. 👍

@crobert-1 Thanks for the quick review! I'll take a look through those shortly.

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

I see I've got a couple open comments still, so I'll wait to review tests until after those are responded to 👍

receiver/awss3receiver/README.md Outdated Show resolved Hide resolved
@adcharre
Copy link
Contributor Author

@crobert-1

I see I've got a couple open comments still, so I'll wait to review tests until after those are responded to 👍

I think I've been through all your original comments and resolved them now.

@atoulme
Copy link
Contributor

atoulme commented Apr 17, 2024

Ready to go!

@dmitryax
Copy link
Member

Hey @adcharre, sorry for the delay with merging. Can you please rebase again?

@adcharre
Copy link
Contributor Author

Hey @adcharre, sorry for the delay with merging. Can you please rebase again?

@dmitryax done!

@adcharre
Copy link
Contributor Author

Rebased again ... @dmitryax any chance this can go in soon while I just love resolving merge conflicts I would like to see this merged :-)

@xThomo
Copy link

xThomo commented Apr 29, 2024

@atoulme / @dmitryax is there a reason holding up the merge of this PR? I see others being merged over the past couple of weeks.

@atoulme
Copy link
Contributor

atoulme commented Apr 29, 2024

@atoulme / @dmitryax is there a reason holding up the merge of this PR? I see others being merged over the past couple of weeks.

No reason that I can think of. I have approved this PR and marked it ready to merge. I cannot merge PRs, maintainers can. They are busy and this is a big PR.

@atoulme
Copy link
Contributor

atoulme commented May 3, 2024

@adcharre I tried to help but I don't think I got it right, mind taking a look?

@adcharre
Copy link
Contributor Author

adcharre commented May 7, 2024

@dmitryax rebased ready for merging

@andrzej-stencel andrzej-stencel merged commit 4ae20e9 into open-telemetry:main May 7, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/awss3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants