Sanitise invalid characters in metric names #55
Conversation
|
Hey @ben-healthforge Thanks for the contribution! Since the dogstatsd clients can typically see a high throughput of data points, one thing I want to be mindful of is any performance impact. Have you happened to see any performance difference before and after this change? Are you currently experiencing metrics with a |
|
Hi @nmuesch, we haven't noticed a performance impact at our level of usage but I suppose it could become significant as the number of metrics increases. Metrics with a |
|
Thanks for the info and context. I'll try to take a look to see if there is some performance impact here, if noticeable we may want to put this behind some configuration option if possible. |
|
According to https://docs.datadoghq.com/developers/faq/what-best-practices-are-recommended-for-naming-metrics-and-tags/ this sanitization is already supposed to happen, or am I misreading it? |
There was a problem hiding this comment.
Hi @ben-healthforge Thanks a lot for your contribution. This is great! However, I may miss something very obvious regarding your logic (see my first comment).
| @@ -427,7 +434,7 @@ String tagString(final String[] tags) { | |||
| */ | |||
| @Override | |||
| public void count(final String aspect, final long delta, final String... tags) { | |||
| send(new StringBuilder(prefix).append(aspect).append(":").append(delta).append("|c").append(tagString(tags)).toString()); | |||
| send(new StringBuilder(prefix).append(sanitiseAspect(aspect)).append(":").append(delta).append("|c").append(tagString(tags)).toString()); | |||
There was a problem hiding this comment.
Could we factorize this logic in a function instead of repeating it almost everywhere?
ben-healthforge
force-pushed
the
feature/sanitise-metric-names
branch
from
fe3372e to
19ce172
Compare
resolves #27