Allow statsd protocol to be configurable#157
Allow statsd protocol to be configurable#157ianbishop wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Ian Bishop <ibishop@flipboard.com>
| UseStatsd bool `envconfig:"USE_STATSD" default:"true"` | ||
| StatsdHost string `envconfig:"STATSD_HOST" default:"localhost"` | ||
| StatsdPort int `envconfig:"STATSD_PORT" default:"8125"` | ||
| StatsdProtocol string `envconfig:"STATSD_PROTOCOL" default:"tcp"` |
There was a problem hiding this comment.
Does setting STATSD_PROTOCOL without making this change not work for you?
(For me it seems to work without this change, just with the declaration in https://github.com/lyft/gostats/blob/f9add79ec5fce9579fb2e52b6b419da2ee965b37/settings.go#L26)
There was a problem hiding this comment.
Although even if it works without this, declaring it here might be useful from a documentation / discoverability perspective
There was a problem hiding this comment.
I did try setting STATSD_PROTOCOL and thought that it did not work which led to this PR. I will try it again, I probably made an error.
There was a problem hiding this comment.
Sorry, the issue seems to be that STATSD_PROTOCOL does not work with the envoyproxy/ratelimit:v1.4.0 container. Building a new container from main works without this change.
Would you like me to close it or leave it for documentation purposes?
There was a problem hiding this comment.
I don't have strong options either way, but also I'm not a maintainer so I wouldn't be able to merge regardless
Seems like removing the statsd stuff (host, port, etc.) from settings.go and just linking from the README to whatever documentation exists in the gostats library would require the least amount of ongoing maintenance
There was a problem hiding this comment.
+1 can we just move this to the README?
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This capability is already available via lyft/gostats, this just exposes it to be configurable in ratelimit. We require this as we only support UDP statsd for performance reasons.
Signed-off-by: Ian Bishop ibishop@flipboard.com