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

[exporter/awss3] feat!: partition format #37954

Merged
merged 22 commits into from
Mar 4, 2025

Conversation

kevthompson
Copy link
Contributor

@kevthompson kevthompson commented Feb 15, 2025

Description

Replacing the limited s3_partition option with a dynamic s3_partition_format option to allow custom filenames. Allows users to define their own partition formats (e.g. removing the "year=" strings or partitioning weekly).

This is a breaking change, I created this as a new input and removed the s3_partition so anyone that was on hourly partitions won't accidentally upgrade and change their file formats. The default format still points to the same location.

Also fixed a bug where not specifying a prefix would create an empty directory in the bucket.

Link to tracking issue

Fixes #37915
Fixes #37503

Testing

Test cases all passing

Tested against personal S3 bucket

  • when no value is given it matches the current default => s3://test-otel-s3-exporter/year=2025/month=02/day=23/hour=22/minute=34/logs_235895800.json
  • custom path '%Y/%m/%d/%H/%M' => s3://test-otel-s3-exporter/2025/02/23/22/34/logs_216971670.json
  • custom path with prefix => s3://test-otel-s3-exporter/baz/2025/02/23/22/33/logs_134704167.json

Documentation

Updated README with new s3_partition_format input & removed the s3_partition entry. Added an extra example config with a custom partition format.

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.

Changes make sense, it's an alpha exporter, maybe we can afford this breaking change.

@kevthompson
Copy link
Contributor Author

Validated against my personal S3 bucket & fixed the link the workflows were complaining about, all working as expected

@atoulme atoulme added ready to merge Code review completed; ready to merge by maintainers and removed ready to merge Code review completed; ready to merge by maintainers labels Feb 26, 2025
@atoulme
Copy link
Contributor

atoulme commented Feb 26, 2025

CI fails:

go.mod/go.sum deps changes detected, please run "make gotidy" and commit the changes in this PR.

@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Feb 28, 2025
@atoulme
Copy link
Contributor

atoulme commented Feb 28, 2025

I am kicking the build, please hold off merging main, I have to kick the build off every time you do :)

// Valid values are: [hour,minute]
S3Partition string `mapstructure:"s3_partition"`
// S3PartitionFormat is used to provide the rollup on how data is written. Uses [strftime](https://www.man7.org/linux/man-pages/man3/strftime.3.html) formatting.
S3PartitionFormat string `mapstructure:"s3_partition_format"`
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this change could be done in a less disruptive manner for users, by deprecating the s3_partition field first and removing it in a future version.

The component is in alpha, so I suppose this is acceptable.

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 see now that there is also a receiver awss3receiver with the same s3_partition: input, do these need to be kept in line?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's alpha and I think we need time to get things right. Yes, we might set the same approach for the s3 receiver.

@andrzej-stencel andrzej-stencel changed the title feat: partition format [exporter/awss3] feat: partition format Mar 3, 2025
@andrzej-stencel andrzej-stencel changed the title [exporter/awss3] feat: partition format [exporter/awss3] feat!: partition format Mar 3, 2025
@andrzej-stencel
Copy link
Member

Some conflicts to resolve @kevthompson

@andrzej-stencel andrzej-stencel removed the ready to merge Code review completed; ready to merge by maintainers label Mar 3, 2025
@atoulme atoulme merged commit 96755c4 into open-telemetry:main Mar 4, 2025
156 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 4, 2025
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.

(Request) [AWS S3 Exporter] Custom File Paths [BUG][exporter/awss3] Newer versions path has extra "/" prefix
4 participants