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

Added method (convertDurationsTo) to convert default reporting units (nanoseconds) to user… #32

Closed
wants to merge 1 commit into from

Conversation

harnitsignalfx
Copy link
Contributor

…-defined Time Unit

@harnitsignalfx
Copy link
Contributor Author

Let me know if this doesn't fit the SFx standards or I need to change a few (or more) things

@mpetazzoni
Copy link
Contributor

I'm 👎 on this.

I understand why people think they want this, but really they shouldn't.

I'm afraid this could cause much more harm than good, especially if multiple emitters don't report the same metrics (with different dimensions) using the same units. It becomes impossible to do any kind of computation across those timeseries because they'll have values in different orders of magnitude.

Time units are like timezones. You want to measure and store in the most absolute form possible (nanoseconds for durations, UNIX timestamps for time), and then have the UI present this data in whatever unit (or timezone) the user prefers most.

@harnitsignalfx
Copy link
Contributor Author

Hmm, I see your point. From the point of view of the customer though, if their value is far too granular to provide value as is, the only alternative left for them is to use Scale on every one of their signalflow jobs within every single chart.
Also, it looks like the duration and unit are available in the default codahale reporter. I'll be making a few changes that Ted mentioned on making this change equivalent to how its typically done by the original reporter.
Having said that - if you feel very strongly about it, then I can close this PR.

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 this pull request may close these issues.

3 participants