stats: DogStatsd sink supports custom metric name prefix#4506
stats: DogStatsd sink supports custom metric name prefix#4506mattklein123 merged 4 commits intoenvoyproxy:masterfrom
Conversation
ref: envoyproxy#4390 Signed-off-by: Taiki Ono <taiki-ono@cookpad.com>
|
@mrice32 do you mind taking a first pass? |
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
This looks awesome! LGTM. One thing I did notice is that as far as I can tell, we don't have any tests that verify that the UDP or TCP sinks add the configured prefix to the metric names that they output. That is somewhat orthogonal to this PR since the statsd prefix feature was preexisting, but just wanted to point it out.
|
|
||
| reserved 2; | ||
|
|
||
| // Optional custom metric name prefix the same as :ref:`StatsdSink's prefix field |
There was a problem hiding this comment.
nit: maybe rephrase to something like:
// Optional custom metric name prefix. See
// :ref:`StatsdSink's prefix field <envoy_api_field_config.metrics.v2.StatsdSink.prefix>`
// for more details.
There was a problem hiding this comment.
Oh, I though I fixed it... I'll fix it now.
There was a problem hiding this comment.
Thanks, now I fixed this.
|
Right, I agree we should have tests in that area. I'll try adding them in this PR. |
|
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! |
stale
bot
added
the
stale
Signed-off-by: Taiki Ono <taiki-ono@cookpad.com>
stale
bot
removed
the
stale
|
Sorry for being late. I added some tests for custom metric prefix. |
|
@taiki45 can you merge master to bring in the bazel build cache fix and also fix merge conflicts? I can look after that. |
|
@mattklein123 Thanks, I merged master and it seems the merge resolved conflicts automarically. |
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
LGTM outside of that one nit above. Thanks for adding those new tests - this is awesome!
|
|
||
| reserved 2; | ||
|
|
||
| // Optional custom metric name prefix the same as :ref:`StatsdSink's prefix field |
Signed-off-by: Taiki Ono <taiki-ono@cookpad.com>
…4506) ref: envoyproxy#4390 Signed-off-by: Taiki Ono <taiki-ono@cookpad.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>
4 participants
Description:
DogStatsd sink supports custom global metric name prefix like StatsdSink does.
Fixes #4390
Risk Level: Low, small optional feature.
Testing: Automated tests passed locally. Manually tested with statsd_exporter and Prometheus.
Docs Changes: Included.
Release Notes: Included.