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/awskinesisexporter] Adding region config #16233

Merged

Conversation

MovieStoreGuy
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy commented Nov 10, 2022

Description:
This was an oversight with the upgrade for the client to use aws sdk v2

Link to tracking Issue:
#16259

Testing:
NA

Documentation:
The documentation already exists.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

This seem to be a bug, so please add a changelog entry

@MovieStoreGuy
Copy link
Contributor Author

Apologies, I did forget it in my very tired state.

@simshengqin
Copy link

@Aneurysm9 any updates regarding this? I urgently require a fix to this issue and will appreciate an update, thank you!

lo.Region = conf.AWS.Region
return nil
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this change tested?

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've been thinking about this, I don't think I have a good answer atm.

Let me play with some ideas on how to expose this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So how I wanted to do this and what came out are very different due to the fact the fact that using assert.EqualValues resulted in differences due to lambda references being stored.

The best alternative without doing much of a major refactor was to create an options type and redefine the kinesis.NewClient method.

@mx-psi
Copy link
Member

mx-psi commented Nov 17, 2022

@codeboten anything left to address here?

@MovieStoreGuy MovieStoreGuy 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 Nov 19, 2022
@MovieStoreGuy MovieStoreGuy force-pushed the msg/chore-include-region branch from c4908ba to 210bf91 Compare November 22, 2022 02:11
@github-actions github-actions bot requested a review from Aneurysm9 November 22, 2022 02:11
@MovieStoreGuy MovieStoreGuy force-pushed the msg/chore-include-region branch from 210bf91 to fde67a9 Compare November 24, 2022 00:41
@MovieStoreGuy MovieStoreGuy added the ready to merge Code review completed; ready to merge by maintainers label Nov 24, 2022
@mx-psi
Copy link
Member

mx-psi commented Nov 24, 2022

ping @codeboten

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @MovieStoreGuy!

@codeboten codeboten merged commit 27f10ce into open-telemetry:main Nov 24, 2022
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
)

Updating kinesis exporter to include fix for setting config
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/awskinesis ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants