Sanitise invalid characters in metric names#55
Sanitise invalid characters in metric names#55ben-healthforge wants to merge 3 commits intoDataDog:masterfrom
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? |
rud
left a comment
rud
left a comment
There was a problem hiding this comment.
Was just checking the codebase for this functionality, glad I found this PR. +1 to merge it 🚀
gzussa
left a comment
gzussa
left a comment
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).
| @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
5 participants
resolves #27