opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

fix(s3exporter/config-validation): endpoint should contain region and S3 bucket

Open dabcoder opened this issue 2 months ago • 4 comments

Description:

Addresses issue #32774. Discussion: https://github.com/open-telemetry/opentelemetry-collector-contrib/discussions/32766.

Still a draft.

Link to tracking Issue: #32774

Testing: added test cases to validate config when endpoint is provided.

Documentation: TBD

Configuration logic (to be reviewed):

  • An endpoint is provided, and Region still needs to be provided (as per the AWS SDK Go config, even though the region can be included in the endpoint itself). The S3 bucket name is optional.
  • An endpoint is not provided, thus region and S3 bucket name should be provided (return an error for each if one of those 2 isn't)

Tasks:

  • [ ] Add entry to changelog
  • [x] Edit the docs?

dabcoder avatar May 02 '24 08:05 dabcoder

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: dabcoder / name: David B. (3d95e5a162a2732f766923a6d977a6ba5c3f621e, 138679459d255abab6d965275366bc2c1fde0cff, c79f3750ad9639a56b8f77df2fa1ec57b16c2370, 5a23120ccee51aaa96f025c667d2d2bb7c8498fd, b29567ca38c68e26b14f95d0bc2c766c57ac04a0, fa5ca3fa494fe279609751338da2da21ae98a60e, 853d91611baa3f8190ef00b9ba1f6aec67196db4, 362a623825387ee92f13990fd524a4356f93b49e, df9b2a0d2213fd269d55053d1ce2a53bc010f386, 7b60ae7e027de11b78c43d49db528949adb9ecba)

Actually I am not sure what protocol endpoint should use. I am seeing unsupported protocol scheme \"s3\""} errors in logs when using an endpoint starting with s3://, so we may also need to clarify that this support http(s) only?

dabcoder avatar May 03 '24 16:05 dabcoder

cc @atoulme @pdelewski.

dabcoder avatar May 07 '24 15:05 dabcoder

Actually I am not sure what protocol endpoint should use. I am seeing unsupported protocol scheme \"s3\""} errors in logs when using an endpoint starting with s3://, so we may also need to clarify that this support http(s) only?

Yes, I guess so.

atoulme avatar May 07 '24 15:05 atoulme

@bogdandrutu anything else I should do in order to merge this?

dabcoder avatar May 14 '24 16:05 dabcoder