-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
base: main
Are you sure you want to change the base?
[exporter/awskinesisexporter] Add stream ARN parameter support (#33891) #35106
Conversation
@@ -18,6 +18,7 @@ import ( | |||
|
|||
type batcher struct { | |||
stream *string | |||
arn *string |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
5643408
to
ab8166d
Compare
ab8166d
to
9ee6064
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…telemetry#33891) Resolves open-telemetry#33891 Signed-off-by: Hong Chen <hong.chen.7219@gmail.com>
9ee6064
to
788b463
Compare
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? |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description:
Add stream ARN parameter support.
Link to tracking Issue:
Resolves #33891
Testing:
Documentation: