opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard
verified publisher icon open-telemetry

s3mapprovider for Collector: load configuration from config files in Amazon S3

Open junhaoyu-aws opened this issue 3 years ago • 12 comments

Description: <Describe what has changed.>

An implementation of ConfigMapProvider for Amazon S3 (s3mapprovider) allows OTEL Collector the ability to load configuration for itself by fetching and reading config files stored in Amazon S3.

Link to tracking Issue: <Issue number if applicable> #12939

Testing: <Describe what testing was performed and which tests were added.> First, I mocked how a S3 client works while interacting with a S3 server. Second, I designed and passed all the unit tests for edge cases such as empty fields, invalid config files, non-exist config files and so on. Third, I did integration tests such as:

  • Start an OTEL Collector with s3mapprovider.
  • Start an OTEL sample application to send telemetry data to check if the OTEL Collector is running well.

Documentation: <Describe the documentation added.> Added a README.md

Sponsor: @Aneurysm9

junhaoyu-aws avatar Aug 09 '22 03:08 junhaoyu-aws

@Aneurysm9 to review

mx-psi avatar Aug 10 '22 07:08 mx-psi

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: junhaoyu-aws / name: Junhao Yu (c794ece41dbe3d905248fce8b5d57293877d2168, 48858d6cbbeb47a0c31d8baf5a08bb1f747cfef5, 02d6b6949544cc3d021d1a901579b38c18b1b8a0, a3f46be907c6d961de1666466a043e8eab574910, be122d92907742e477f6387c42caa2f606770b24, 253f4d61798a742ef73dc9ac6148d8c7ae59fa2b, 07429c1adc79edc002a484c60e948e7e43c96a01, 50e1b9cfd413e2bf5e3820e32945db64ea9f000d, bb222efda87c609e72fadded51befba70f023970, db01c86126aedafa975d52b2a3283f4fd7016410, 09f1dcb85aaeb5d457b43d773682a2f1290f516d, b8b0d0d90931943a7c370ffacfd9f82aa8607d4f, bf3735b53c322aa40ac6bd0a01a7e6904c5f2a2c, b0d268290bb6d8c89ea9ce627eb668b060793f82, 2c4054a03e564eda097f7119f19fc60e2b98c183)
  • :white_check_mark: login: rice-junhaoyu / name: rank1_coder (d665128a07e8c628e241902bc33aa3ce3e9eca0e, 11101f7498f3de934cf3c166378c2824102c2af7)

linux-foundation-easycla[bot] avatar Aug 18 '22 03:08 linux-foundation-easycla[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Sep 01 '22 05:09 github-actions[bot]

/remove-label stale

jeromeinsf avatar Sep 01 '22 21:09 jeromeinsf

@mx-psi, is this still missing something, or can it be merged?

jpkrohling avatar Sep 02 '22 12:09 jpkrohling

CLA Missing ID CLA Not Signed

  • :white_check_mark: login: junhaoyu-aws / name: Junhao Yu (02d6b6949544cc3d021d1a901579b38c18b1b8a0, a3f46be907c6d961de1666466a043e8eab574910, 253f4d61798a742ef73dc9ac6148d8c7ae59fa2b, 50e1b9cfd413e2bf5e3820e32945db64ea9f000d, db01c86126aedafa975d52b2a3283f4fd7016410, b8b0d0d90931943a7c370ffacfd9f82aa8607d4f, b0d268290bb6d8c89ea9ce627eb668b060793f82, 2c4054a03e564eda097f7119f19fc60e2b98c183)
  • :white_check_mark: login: rice-junhaoyu / name: rice-junhaoyu (d665128a07e8c628e241902bc33aa3ce3e9eca0e, 11101f7498f3de934cf3c166378c2824102c2af7, c858b12ae018569ec30e6e42bd952dae9d91ea98, a9824f84673bebf816c74b30aa33311c3ef0ece9, 5d27f8bc6f6a41c1b6400577e1950c8ca77c5706)
  • :x: The commit (c794ece41dbe3d905248fce8b5d57293877d2168, 48858d6cbbeb47a0c31d8baf5a08bb1f747cfef5, be122d92907742e477f6387c42caa2f606770b24, 07429c1adc79edc002a484c60e948e7e43c96a01, bb222efda87c609e72fadded51befba70f023970, 09f1dcb85aaeb5d457b43d773682a2f1290f516d, bf3735b53c322aa40ac6bd0a01a7e6904c5f2a2c). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

linux-foundation-easycla[bot] avatar Sep 16 '22 02:09 linux-foundation-easycla[bot]

@rice-junhaoyu @Aneurysm9 there are two things to fix for merging this:

  1. Add a CODEOWNER entry for this provider (I assume this would be someone from AWS?)
  2. Sign the CLA for the commits by @rice-junhaoyu

mx-psi avatar Sep 16 '22 08:09 mx-psi

@rice-junhaoyu @Aneurysm9 there are two things to fix for merging this:

  1. Add a CODEOWNER entry for this provider (I assume this would be someone from AWS?)
  2. Sign the CLA for the commits by @rice-junhaoyu

@mx-psi The @rice-junhaoyu has signed CLA yet. I think the current issue is related to my deactivated AWS email address. The commits are pushed when I was interning at AWS.

I referred to this link and figured out I need to add the AWS email address in my GitHub account's emails. But the problem is I am not able to use my AWS email address any more. Also, the old commits probably seem not being linked any more according to the link.

Do you have any idea on this, or I can restart a PR for this s3provider?

rice-junhaoyu avatar Sep 16 '22 15:09 rice-junhaoyu

@rice-junhaoyu @Aneurysm9 there are two things to fix for merging this:

  1. Add a CODEOWNER entry for this provider (I assume this would be someone from AWS?)
  2. Sign the CLA for the commits by @rice-junhaoyu

@mx-psi The @rice-junhaoyu has signed CLA yet. I think the current issue is related to my deactivated AWS email address. The commits are pushed when I was interning at AWS.

Ah, you are right, got it!

I referred to this link and figured out I need to add the AWS email address in my GitHub account's emails. But the problem is I am not able to use my AWS email address any more. Also, the old commits probably seem not being linked any more according to the link.

Do you have any idea on this, or I can restart a PR for this s3provider?

The nuclear option is to

  1. run git rebase -i $(git merge-base main HEAD),
  2. squash all commits from your previous account into a single one (resulting in a new commit done by your new account). This should be a matter of following the instructions for squash in the editor screen that will come up.
  3. Finish the rebase (you may need to do git rebase --continue)
  4. Force push into this branch (git push --force-with-lease)

If that sounds too hard, opening a new PR is also an option of course :)

mx-psi avatar Sep 16 '22 16:09 mx-psi

@mx-psi I tried to squash all commits onto my first commit c974ece, which is a single commit. But there are about 100 commits included when I called git rebase -i c974ece, maybe since I have merged latest commits from upstream to my fork before. On my understanding, this will lead to a case that those unrelated commits are also included in the file changes of this PR? That will be really ugly. :(

If I am right, would you recommend me to open a new PR instead?

rice-junhaoyu avatar Sep 16 '22 21:09 rice-junhaoyu

If I am right, would you recommend me to open a new PR instead?

@rice-junhaoyu Yes, new PR also works, please ping me and mention this PR so that people understand this code has already been reviewed. Thanks for taking the time to make this change and sorry about the hassle

mx-psi avatar Sep 19 '22 10:09 mx-psi

If I am right, would you recommend me to open a new PR instead?

@rice-junhaoyu Yes, new PR also works, please ping me and mention this PR so that people understand this code has already been reviewed. Thanks for taking the time to make this change and sorry about the hassle

@mx-psi Thanks for your understanding. So sorry about this, I didn't expect my deactivated AWS email address would lead to this situation.

I've already opened a new PR for this and ping you yet.

rice-junhaoyu avatar Sep 20 '22 01:09 rice-junhaoyu

Replaced by https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/14317

codeboten avatar Sep 26 '22 15:09 codeboten