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] Add stream ARN parameter support (#33891) #35106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HongChenTW
Copy link

Description:

Add stream ARN parameter support.

Link to tracking Issue:

Resolves #33891

Testing:

Documentation:

@@ -18,6 +18,7 @@ import (

type batcher struct {
stream *string
arn *string
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be used together or should they be seperate?

Copy link
Author

Choose a reason for hiding this comment

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

According to the AWS document, both ways are valid:

When invoking this API, you must use either the StreamARN or the StreamName parameter, or both. It is recommended that you use the StreamARN input parameter when you invoke this API.

ref: https://docs.aws.amazon.com/kinesis/latest/APIReference/API_DescribeStream.html

@@ -14,6 +14,7 @@ import (
// AWSConfig contains AWS specific configuration such as awskinesis stream, region, etc.
type AWSConfig struct {
StreamName string `mapstructure:"stream_name"`
StreamARN string `mapstructure:"stream_arn"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there any validation for this config?

Copy link
Author

Choose a reason for hiding this comment

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

I was also considering of adding validations, but decided to leave the validation to AWS api, since stream_name also has rule of the length and pattern.

Length Constraints: Minimum length of 1. Maximum length of 128.
Pattern: [a-zA-Z0-9_.-]+

But different from the ARN, AWS api will return the clear error message with invalid stream name:

at 'streamName' failed to satisfy constraint: Member must satisfy regular expression pattern: [a-zA-Z0-9_.-]+

And the ARN only shows:

The input ARN "arn:aws:kinesis:xxx:0:stream/invalid-stream$" is invalid

Base on the validation styles in Opentelemetry collector projects, do you think we should do the ARN validation?
The implementation is clear and easy to add: https://pkg.go.dev/github.com/aws/aws-sdk-go-v2@v1.30.4/aws/arn#IsARN


reference: https://docs.aws.amazon.com/kinesis/latest/APIReference/API_DescribeStream.html#API_DescribeStream_RequestSyntax

@HongChenTW HongChenTW force-pushed the awskinesisexporter-add-arn-config branch from 5643408 to ab8166d Compare September 20, 2024 00:43
@HongChenTW HongChenTW requested a review from a team as a code owner September 20, 2024 00:43
@HongChenTW HongChenTW force-pushed the awskinesisexporter-add-arn-config branch from ab8166d to 9ee6064 Compare September 24, 2024 13:46
Copy link
Contributor

github-actions bot commented Oct 9, 2024

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 9, 2024
@HongChenTW HongChenTW force-pushed the awskinesisexporter-add-arn-config branch from 9ee6064 to 788b463 Compare October 10, 2024 15:27
@HongChenTW
Copy link
Author

Hi @MovieStoreGuy, since the original request seems not an issue to feature requester anymore, although I already had some implementation on this, should we continue or close this PR?

@github-actions github-actions bot removed the Stale label Oct 11, 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 Oct 25, 2024
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.

[awskinesisexporter] [RESOLVED] Cross account support
3 participants