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

aws s3 exporter second PR, implementation #10000

Closed
wants to merge 3 commits into from

Conversation

pdelewski
Copy link
Member

@pdelewski pdelewski commented May 13, 2022

Description:
awss3exporter exports logs and traces

Link to tracking Issue:
#2835
Testing:
Unit tests covering exporter code
Documentation:
Readme describing exporter functionality and configuration.

@pdelewski pdelewski requested review from a team and djaglowski May 13, 2022 12:41
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pdelewski / name: Przemyslaw Delewski (b0a713f, 15a612e75e9f0609102b753caa7bee9d2d2f0bee)

@pdelewski pdelewski changed the title aws s3 exporter implementation aws s3 exporter second PR, implementation May 13, 2022
@pdelewski pdelewski force-pushed the s3exporter-2 branch 8 times, most recently from 1cf1f74 to 15a612e Compare May 17, 2022 16:06
@pdelewski pdelewski force-pushed the s3exporter-2 branch 7 times, most recently from 1e6f853 to 3ddf560 Compare June 6, 2022 12:10
@pdelewski pdelewski force-pushed the s3exporter-2 branch 7 times, most recently from 208d0b3 to bfe1d9b Compare June 7, 2022 12:57
@thomasbaldwin
Copy link
Contributor

can we get this merged in?

@thomasbaldwin
Copy link
Contributor

and how could I test this out locally?

@pdelewski
Copy link
Member Author

can we get this merged in?

first, it needs to be reviewed

year, month, day := time.Date()
hour, minute, _ := time.Clock()

rand.Int()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what's the purpose of this line

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be leftover

@@ -0,0 +1,31 @@
# AWS S3 Exporter for OpenTelemetry Collector
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some discussion (or example) on partitioning could be helpful here

Copy link
Member Author

Choose a reason for hiding this comment

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

right, will add it


func getS3Key(time time.Time, bucket string, keyPrefix string, partition string, filePrefix string, metadata string, fileformat string) string {
timeKey := getTimeKey(time, partition)
randomID := rand.Int()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if that matters much, but perhaps it would be nice to have padded number (so always the same number of characters). Also, some encoding (even base64) could be used to shorten the random part. S3 has max object length of 1024 so we should be well within the limit, but, just in case, might be worth considering

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@thomasbaldwin
Copy link
Contributor

@pdelewski is it possible to provide multiple buckets to export to? one of the reasons for this is because we normally provide a backup bucket for logs in case there is an outage with one region, we have another region & bucket to drop logs into

@pdelewski
Copy link
Member Author

@pdelewski is it possible to provide multiple buckets to export to? one of the reasons for this is because we normally provide a backup bucket for logs in case there is an outage with one region, we have another region & bucket to drop logs into

@thomasbaldwin Sure, but this requires configuration refactoring/extension. I will think about it.

@thomasbaldwin
Copy link
Contributor

@pdelewski is it possible to provide multiple buckets to export to? one of the reasons for this is because we normally provide a backup bucket for logs in case there is an outage with one region, we have another region & bucket to drop logs into

@thomasbaldwin Sure, but this requires configuration refactoring/extension. I will think about it.

sounds good @pdelewski, thanks for all your work on this. And regarding the path name, we generally have all our logging structured in such a way that the root folder name is usually "AWSLogs" within the bucket. Not a blocker by any means but would be nice to be able to control that

@github-actions
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 Jul 15, 2022
@thomasbaldwin
Copy link
Contributor

Can we merge this? I’m using this on staging environments and it runs without issue

@github-actions github-actions bot removed the Stale label Jul 16, 2022
Copy link
Contributor

@thomasbaldwin thomasbaldwin left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

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 Aug 9, 2022
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot closed this Sep 27, 2022
metric/year=XXXX/month=XX/day=XX/hour=XX/minute=XX
```

## AWS Credential Configuration

Choose a reason for hiding this comment

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

About AWS Credentials config, i think need more example

@pdelewski pdelewski mentioned this pull request Apr 7, 2023
@atoulme atoulme reopened this Apr 15, 2023
@atoulme atoulme self-requested a review as a code owner April 15, 2023 05:35
@atoulme
Copy link
Contributor

atoulme commented Apr 15, 2023

Given the conflicts, I have opened #20912 instead to follow up based on the work done in this PR. Closing this PR for good.

@atoulme atoulme closed this Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants