Consolidate AWS credentials, Cloudwatch dimension wilcards #1155
Conversation
| Region string `toml:"region"` // AWS Region | ||
| AccessKey string `toml:"access_key"` // Explicit AWS Access Key ID | ||
| SecretKey string `toml:"secret_key"` // Explicit AWS Secret Access Key | ||
| RoleArn string `toml:"role_arn"` // Role ARN to assume |
There was a problem hiding this comment.
These toml tags aren't actually supported because they are on an embedded, anonymous struct.
I've created a PR to add that support here - influxdata/toml#6
|
Thanks @johnrengelman, probably won't be able to squeeze this into 0.13 as I'm OOO for the next few days, @ljosa & @joshglympse if you have time to review I'd appreciate it a lot! |
| Period internal.Duration `toml:"period"` | ||
| Delay internal.Duration `toml:"delay"` | ||
| Namespace string `toml:"namespace"` | ||
| Metrics []*Metric `toml:"metrics"` | ||
| CacheTTL string `toml:"cache_ttl"` |
There was a problem hiding this comment.
Why not use an internal.Duration for this to handle the duration string parsing?
There was a problem hiding this comment.
Also, it looks like you made this a required config value - can you add to the sample config and README?
There was a problem hiding this comment.
I'll switch it to Duration.
| @@ -153,22 +173,19 @@ func (c *CloudWatch) Gather(acc telegraf.Accumulator) error { | |||
|
|
|||
| func init() { | |||
| inputs.Add("cloudwatch", func() telegraf.Input { | |||
| return &CloudWatch{} | |||
| return &CloudWatch{ | |||
| CacheTTL: "1hr", | |||
There was a problem hiding this comment.
@joshhardy - this configures the default value to what it was before.
It's not required in the config file, if unspecified, it will be this.
|
@johnrengelman seems like you could accomplish this without embedded fields, which are confusing to me, because it requires guessing where the fields in the config file are going and then digging into different dependencies trying to find them. You could design it by putting each of those fields on the different AWS plugins, and then instantiating an influxaws credential object using those. This is more explicit and I think easier for future debugging. |
| ## 3) shared profile from 'profile' | ||
| ## 4) environment variables | ||
| ## 5) shared credentials file | ||
| ## 6) EC2 Instance Profile |
There was a problem hiding this comment.
since we have example fields for the keys, should you put
#role_arn = ""
here?
|
@sparrc that's a good suggestion (still learning the best practices with Go). I'll refactor this PR to do that. |
sparrc
force-pushed
the
release-0.13
branch
5 times, most recently
from
69ad61b
to
12357ee
Compare
|
Closing this PR now that 0.13 is release and reopened against |
Wasn't sure if you'd be ok with trying to squeeze this into 0.13 or not (I can retarget it to master if you'd like).
This PR does 3 things:
Closes #1115, #1101