-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: select db instances by tags #232
base: main
Are you sure you want to change the base?
Conversation
428937a
to
1ff1103
Compare
3a9c673
to
cff32d0
Compare
I thought I was basically finished with this PR, but during testing I realised serious limitations in tag selection since configuration of prometheus-rds-exporter is done using viper which isn't case sensitive: spf13/viper#1014 This means that you can only select tags where keys and values are lower case, which isn't a convention most use. I don't see how to get around this issue while keeping viper. Maybe switch to koanf? |
d30e1d3
to
f3c1a27
Compare
So, that's what I did: switched to koanf for configuration |
f3c1a27
to
40ac261
Compare
We'll have a look. 👀 |
I now realised that so far I haven't implemented any limitation of which metrics are fetched from CloudWatch. I can look into that if you like. |
Ah, it's only the rds_usage_* series that are fetched as the sum for all instances. But I don't think they can be fetched from CloudWatch any other way, so I think it's fine. |
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.
Hello @msvticket.
Thanks for this contribution! This feature is definitely needed!
I've added some recommendations as comment
@@ -335,7 +336,7 @@ func TestInstanceAge(t *testing.T) { | |||
ctx := context.TODO() | |||
client := mock.RDSClient{DescribeDBInstancesOutput: mockDescribeDBInstancesOutput} | |||
configuration := rds.Configuration{} | |||
fetcher := rds.NewFetcher(ctx, client, configuration) | |||
fetcher := rds.NewFetcher(ctx, client, nil, slog.Logger{}, configuration) | |||
metrics, err := fetcher.GetInstancesMetrics() | |||
|
|||
expectedAge := time.Since(*rdsInstance.InstanceCreateTime) |
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.
Could you add a test case for tag selection please?
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.
What would I test? The selection of resources based on tags are made at AWS...
mostly to get around spf13/viper#1014 Signed-off-by: Mårten Svantesson <[email protected]>
Signed-off-by: Mårten Svantesson <[email protected]>
needed for more complex configuration, i.e. tag-selections Signed-off-by: Mårten Svantesson <[email protected]>
Signed-off-by: Mårten Svantesson <[email protected]>
Signed-off-by: Mårten Svantesson <[email protected]>
40ac261
to
374ca21
Compare
fixes #94