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

Add s3 storage class to the AWS S3 exporter #35574

Merged
merged 12 commits into from
Jan 30, 2025

Conversation

pollosp
Copy link
Contributor

@pollosp pollosp commented Oct 3, 2024

Description: Add s3 storage class

Link to tracking Issue: #35173

Testing: Unit for the config interface

Documentation: Added storage class for the S3

@pollosp pollosp requested review from atoulme and a team as code owners October 3, 2024 13:21
@pollosp pollosp closed this Oct 3, 2024
@pollosp pollosp deleted the S3CLASS branch October 3, 2024 13:22
@github-actions github-actions bot requested a review from pdelewski October 3, 2024 13:23
@pollosp pollosp restored the S3CLASS branch October 3, 2024 13:23
@pollosp pollosp reopened this Oct 3, 2024
@pollosp pollosp changed the title Add s3 storage class Add s3 storage class to the AWS S3 exporter Oct 3, 2024
@hoazgazh
Copy link

hoazgazh commented Oct 9, 2024

im still waiting :D

@scila1996
Copy link

"I’m really looking forward to this feature release as soon as possible. Please review it soon, @atoulme."

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Oct 24, 2024
@pollosp
Copy link
Contributor Author

pollosp commented Oct 24, 2024

@atoulme anything else required for merge this?

@github-actions github-actions bot removed the Stale label Oct 25, 2024
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Nov 22, 2024
@atoulme atoulme removed the Stale label Dec 6, 2024
@@ -32,6 +32,7 @@ The following exporter configuration parameters are supported.
| `encoding_file_extension` | file format extension suffix when using the `encoding` configuration option. May be left empty for no suffix to be appended. | |
| `endpoint` | (REST API endpoint) overrides the endpoint used by the exporter instead of constructing it from `region` and `s3_bucket` | |
| `s3_force_path_style` | [set this to `true` to force the request to use path-style addressing](http://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html) | false |
| `storage_class` | S3 storageclass | STANDARD
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the allowed values for this config key? Is it a known enumeration? Is there a link to docs? Any validation we can perform?

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue (#35173) mentions STANDARD_IA, is STANDARD valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added documentation about the storage class valid APIs and add the all possible options

@atoulme
Copy link
Contributor

atoulme commented Dec 6, 2024

Please rebase off latest main. Please add a changelog entry.

@pollosp
Copy link
Contributor Author

pollosp commented Dec 10, 2024

Please rebase off latest main. Please add a changelog entry.

@atoulme which changelog ?

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Dec 25, 2024
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 9, 2025
@hoazgazh
Copy link

hoazgazh commented Jan 9, 2025

Please reopen 😭

@atoulme atoulme reopened this Jan 9, 2025
@pollosp pollosp force-pushed the S3CLASS branch 2 times, most recently from 80386bb to 968e5dd Compare January 20, 2025 08:02
@pollosp
Copy link
Contributor Author

pollosp commented Jan 20, 2025

@atoulme are QueueSettings defaults in place ?

After rebasing with master have I seen this.

currently:

expected: &awss3exporter.Config{QueueSettings:internal.QueueConfig{Enabled:false, NumConsumers:0, QueueSize:0, Blocking:false, StorageID:(*component.ID)(nil)}, S3Uploader:awss3exporter.S3UploaderConfig{Region:"us-east-1", S3Bucket:"foo", S3Prefix:"bar", S3Partition:"minute", FilePrefix:"", Endpoint:"http://endpoint.com", RoleArn:"", S3ForcePathStyle:false, DisableSSL:false, StorageClass:"STANDARD_IA", Compression:""}, MarshalerName:"otlp_json", Encoding:(*component.ID)(nil), EncodingFileExtension:""}

actual  : &awss3exporter.Config{QueueSettings:internal.QueueConfig{Enabled:false, NumConsumers:10, QueueSize:1000, Blocking:false, StorageID:(*component.ID)(nil)}, S3Uploader:awss3exporter.S3UploaderConfig{Region:"us-east-1", S3Bucket:"foo", S3Prefix:"bar", S3Partition:"minute", FilePrefix:"", Endpoint:"http://endpoint.com", RoleArn:"", S3ForcePathStyle:false, DisableSSL:false, StorageClass:"STANDARD_IA", Compression:""}, MarshalerName:"otlp_json", Encoding:(*component.ID)(nil), EncodingFileExtension:""}

Specifically:

internal.QueueConfig{Enabled:false, NumConsumers:10, QueueSize:1000, Blocking:false, StorageID:(*component.ID)(nil)}

Enabled:false is OK by default , but NumConsumers:10, QueueSize:1000 shouldn't be 0 by default ? or is this in this way to have those values if enabled=true by default ?

I fixed initialising queueCfg in my text , but not sure if the proper way to go.

This means any test must initialise QueueSettings: queueCfg, to have the proper values?

@atoulme
Copy link
Contributor

atoulme commented Jan 20, 2025

yes your expected default config should have the default queue settings values.

@pollosp
Copy link
Contributor Author

pollosp commented Jan 20, 2025

Fixed the test I left them blank , thanks to the lintern I catch it , sorry for the noise , first PR here.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Just a couple nits

@@ -68,6 +70,10 @@ func (c *Config) Validate() error {
if c.S3Uploader.S3Bucket == "" && c.S3Uploader.Endpoint == "" {
errs = multierr.Append(errs, errors.New("bucket or endpoint is required"))
}
if c.S3Uploader.StorageClass == "" {
c.S3Uploader.StorageClass = "STANDARD"
Copy link
Member

Choose a reason for hiding this comment

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

The validate method should not modify the config. This should be set in the factory's CreateDefaultConfig.

We seem to be missing actual validation here too. What if the user enters an invalid value?

| `s3_force_path_style` | [set this to `true` to force the request to use path-style addressing](http://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html) | false |
| `storage_class` | [S3 storageclass](https://docs.aws.amazon.com/AmazonS3/latest/userguide/sc-howtoset.html) | STANDARD
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `storage_class` | [S3 storageclass](https://docs.aws.amazon.com/AmazonS3/latest/userguide/sc-howtoset.html) | STANDARD

Duplicate

bucket string
builder *PartitionKeyBuilder
uploader *manager.Uploader
storageClass string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
storageClass string
storageClass s3types.StorageClass

@@ -26,6 +26,7 @@ func TestNewS3Manager(t *testing.T) {
"my-bucket",
&PartitionKeyBuilder{},
s3.New(s3.Options{}),
"STANDARD", // <-- Add the new argument here
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"STANDARD", // <-- Add the new argument here
"STANDARD",

@@ -139,6 +153,7 @@ func TestS3ManagerUpload(t *testing.T) {
BaseEndpoint: aws.String(s.URL),
Region: "local",
}),
"STANDARD_IA", // <-- Pass your desired storage class here
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"STANDARD_IA", // <-- Pass your desired storage class here
"STANDARD_IA",

@pollosp
Copy link
Contributor Author

pollosp commented Jan 23, 2025

Thanks I will try to add your feedback between this Friday and Monday

@pollosp
Copy link
Contributor Author

pollosp commented Jan 28, 2025

@atoulme and @djaglowski should I remove this lintern directive //nolint:staticcheck is unused for linter "staticcheck" (nolintlint) ?

@djaglowski
Copy link
Member

djaglowski commented Jan 28, 2025

@atoulme and @djaglowski should I remove this lintern directive //nolint:staticcheck is unused for linter "staticcheck" (nolintlint) ?

I'm not familiar with the lint error but I think we just need the lint check to pass so feel free to experiment.

@djaglowski djaglowski merged commit ff2d5e2 into open-telemetry:main Jan 30, 2025
163 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 30, 2025
chengchuanpeng pushed a commit to chengchuanpeng/opentelemetry-collector-contrib that referenced this pull request Feb 8, 2025
**Description:**  Add s3 storage class
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

**Link to tracking Issue:**
open-telemetry#35173

**Testing:**  Unit for the config interface

**Documentation:**  Added storage class for the S3
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.

6 participants