Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete ":" from metric names to allow statsd server insert them #27

Open
odin-delrio opened this issue Mar 9, 2017 · 4 comments · May be fixed by #55
Open

Delete ":" from metric names to allow statsd server insert them #27

odin-delrio opened this issue Mar 9, 2017 · 4 comments · May be fixed by #55
Assignees

Comments

@odin-delrio
Copy link

odin-delrio commented Mar 9, 2017

(Also opened for the original java statsd client at tim-group/java-statsd-client#46)

Hi all!

I'm currently using statsd to automatically record all the metrics provided by some spring boot application.
In my case, spring is creating some metrics with ":" in the name (for some hystrix with feign stats), and these ones are skipped by the statsd server (https://github.com/etsy/statsd).

That implementation allow to insert multiple metric types to the same metric:

$ echo "xxx:1|c:3|g" | nc -u -w0 127.0.0.1 8125 && echo "counters" | nc localhost 8126 && echo "gauges" | nc localhost 8126

Produces this output:

{ 'statsd.bad_lines_seen': 0,
  'statsd.packets_received': 1,
  'statsd.metrics_received': 1,
  xxx: 1 }
END

{ xxx: 3 }
END

This (undocumented) statsd feature does not allow me to make a PR to statsd repository... So, That's why I'm here,
would be acceptable to make you a PR (or request for the change) to simply delete the ":" in the metric names?

The statsd server is already deleting some characters from metric name:
https://github.com/etsy/statsd/blob/master/stats.js#L170

@masci
Copy link
Contributor

masci commented Mar 13, 2017

Hi @odin-delrio,

can you tell me more about your current setup? If you're using java-dogstatsd as a statsd client, why can't you just strip the : on your code before sending over the metric?

@odin-delrio
Copy link
Author

odin-delrio commented Mar 14, 2017

Hi @masci

I can escape them in my own code, in fact, I'm already doing it overriding the needed Spring Bean...
I could make a PR to Spring Boot Actuator to change how they interact with the statsd client, but, I was thinking that fixing this here will be nice for all statsd client users that are not using Spring Boot.

Anyway, I thought that this statsd client and the original one were interchangeable (both are under same package names) but this is not currently true...

Spring is relying in this constructor (you can see that here) which is not currently available in DataDog implementation of the statsd client. Sorry for not test this implementation with Spring boot before open the issue, I assumed that it was compatible...

For summarize, what do you think about:
1 - Add a new constructor to DataDog Statsd client to make it compatible with the original one.

public NonBlockingStatsDClient(String prefix, String hostname, int port, StatsDClientErrorHandler errorHandler) throws StatsDClientException

2 - Escape in the Statsd client the ":" in the metric names avoiding to deal with that to every users of the library.

@robinst
Copy link
Contributor

robinst commented Jul 5, 2017

Looks like this has been fixed in spring-boot. Can this issue be closed?

@odin-delrio
Copy link
Author

I fixed there in spring yes, but if this is done here every user (not only the spring-boot ones) can take the benefit of this...

ben-healthforge added a commit to healthefrog/java-dogstatsd-client that referenced this issue Sep 10, 2018
@ben-healthforge ben-healthforge linked a pull request Sep 10, 2018 that will close this issue
ben-healthforge added a commit to healthefrog/java-dogstatsd-client that referenced this issue Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants