Skip to content

Add detection for IPv6 addresses in Trace#fromRequest#38

Merged
Kami merged 15 commits intotryfer:masterfrom
cjthompson:master
Apr 13, 2016
Merged

Add detection for IPv6 addresses in Trace#fromRequest#38
Kami merged 15 commits intotryfer:masterfrom
cjthompson:master

Conversation

@cjthompson
Copy link
Copy Markdown
Contributor

This patch is based on the other pull request to not crash on an IPv6 address. With this patch, node-tryfer is now compatible with node 4.x and 5.x. I fixed the linting issues from the previous patch and leverage the node built-in net.isIPv4 function with a fallback to a custom implementation.

One additional tweak I added was to check for IPv6 mapped IPv4 addresses and extract the IPv4 address:

    // Check IPv4 mapped IPv6 addresses
    if (/^::ffff:(\d{1,3}\.){3,3}\d{1,3}$/.test(host.address)) {
      host.address = host.address.replace(/^::ffff:/, '');
    }

Niels Nielsen and others added 15 commits September 9, 2015 16:00
…t on the Trace and being passed down into the child spans as well.

Updated existing Trace tests to make sure that the debug property stays undefined when not explicitly set to true
Fix up the leaked global variables
…uffers. Convert the binary annotation values into strings for test comparisons
@9point6
Copy link
Copy Markdown

9point6 commented Jan 14, 2016

Is there any recent progress on getting this merged?

@rooftopsparrow
Copy link
Copy Markdown

Bump.

@Kami
Copy link
Copy Markdown
Contributor

Kami commented Apr 13, 2016

I quickly looked over the changes and they look good to me.

Proper review would be better though, but I don't know when I will be able to get to it. Having said that, we are happy to give write access to new people who would like to help and maintain the project.

Comment thread lib/trace.js
* Returns true if an IP address is v4 dotted decimal
* @param {String} address
* @returns {Boolean}
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some unit tests for this function would also be good.

@Kami Kami merged commit cca67a6 into tryfer:master Apr 13, 2016
@Kami
Copy link
Copy Markdown
Contributor

Kami commented Apr 13, 2016

Sorry for the delay.

I did a more in-depth review and the changes looked good to me so I went ahead and merged them into master.

Sorry for the delay again and thanks a lot for this great 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.

4 participants