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

Support for publishing metrics of BigInteger and BigDecimal type #933

Merged
merged 1 commit into from Apr 8, 2016

Conversation

Projects
None yet
3 participants
@nurkiewicz
Contributor

nurkiewicz commented Apr 8, 2016

No description provided.

@jplock jplock added the improvement label Apr 8, 2016

@jplock jplock added this to the 3.1.3 milestone Apr 8, 2016

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Apr 8, 2016

Member

Seems like a simple additional to me, thanks!

Member

jplock commented Apr 8, 2016

Seems like a simple additional to me, thanks!

@jplock jplock merged commit 6b3a014 into dropwizard:3.1-maintenance Apr 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Apr 8, 2016

Member

Actually I have a problem with this. I don't mind supporting BigInteger and BigDecimal, but this is the wrong way to implement it. BigDecimal -> long discards the fractional part, and both conversions return only the low order 64-bits. Graphite stores everything as doubles, so I think we ought to be converting both BigInteger and BigDecimal to doubles.

Member

ryantenney commented Apr 8, 2016

Actually I have a problem with this. I don't mind supporting BigInteger and BigDecimal, but this is the wrong way to implement it. BigDecimal -> long discards the fractional part, and both conversions return only the low order 64-bits. Graphite stores everything as doubles, so I think we ought to be converting both BigInteger and BigDecimal to doubles.

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Apr 8, 2016

Member

Ahh ok, I reverted the commit, but we'll need another PR to address @ryantenney's comments @nurkiewicz. Sorry about that.

Member

jplock commented Apr 8, 2016

Ahh ok, I reverted the commit, but we'll need another PR to address @ryantenney's comments @nurkiewicz. Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment