Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

fixing hexdec on a 16 char hex number, which could actually exceed INT64 #8

Merged
merged 29 commits into from
Oct 9, 2018
Merged

Conversation

dragoscirjan
Copy link
Contributor

On a 16 'digits' hex chunck, i.e. bea15bfc5e308b1a, hexdec method can trigger erros, because the converted number could exceed PHP integer side.

A solution would be to create a _hexdec method using php-bc or php-gpm to convert the chunk to a decimal number, then convert the number to a 64 bit signed INT number and parse it with intval.

This is my approach, it's not the best, so hoping you guys will refine it further :)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot
Copy link

CLAs look good, thanks!

@dragoscirjan
Copy link
Contributor Author

Need to fix some things before getting back with this PR.

@dragoscirjan dragoscirjan reopened this Oct 5, 2018
@bogdandrutu bogdandrutu requested a review from chingor13 October 5, 2018 14:06
Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this!

src/Jaeger/SpanConverter.php Outdated Show resolved Hide resolved
src/Installer.php Outdated Show resolved Hide resolved
@dragoscirjan
Copy link
Contributor Author

Hi @chingor13,
Please see code changes for the suggestions you have made. I think this is the best form this could be done, for now.

Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The refactoring looks good so far. Just added a few smallish things.

src/JaegerExporter.php Show resolved Hide resolved
* This class handles converting from the OpenCensus data model into its
* Jaeger Thrift representation.
*/
interface SpanConverterInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this interface is needed. There's only one implementation and its internal (or probably should be) until the need arises for a separate implementation.

src/Jaeger/HexdecConverter.php Show resolved Hide resolved
src/Jaeger/HexdecConverterBcMath.php Show resolved Hide resolved
namespace OpenCensus\Trace\Exporter\Jaeger;

/**
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fill this out.

Copy link
Member

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@chingor13 chingor13 merged commit e87763b into census-ecosystem:master Oct 9, 2018
@un000
Copy link

un000 commented Nov 23, 2018

@chingor13 could you make a release for this PR?

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants