Fix endianness of Jaeger IDs for transmission

[ThomsonTan]

Fixes #831

Changes

Detect the host endianness and convert all IDs to big endian if needed.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[maxgolov]

I'm wondering if this is something that should be handled by htons?

https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-htons
https://linux.die.net/man/3/htons

[maxgolov]

Or, to be more precise, something like this?

#define htonll(x) ((1==htonl(1)) ? (x) : ((uint64_t)htonl((x) & 0xFFFFFFFF) << 32) | htonl((x) >> 32))
#define ntohll(x) ((1==ntohl(1)) ? (x) : ((uint64_t)ntohl((x) & 0xFFFFFFFF) << 32) | ntohl((x) >> 32))

[lalitb]

nicely done. this will check the endianness too before performing conversion

[ThomsonTan]

Thanks. I considered htonl as the first option, but I'd like to avoid the runtime check on endianness and preferred the compiler intrinsic to perform byte order swap for simplicity and slight perf win.

[ThomsonTan]

I could change the function name to to_big_endian and define it as as direct return for big endian host. In this way, we don't need check the endianness at callsite of this function.

[ThomsonTan]

Second thought, leave the JAEGER_IS_LITTLE_ENDIAN in the callsite could save us to create bswap_128.

[maxgolov]

I was wondering if it's something you can move into a separate static inline utility function, then it can be reused elsewhere? I think it might be common requirement, to transmit the buffer in big endian, not unique to Jaeger?

[ThomsonTan]

So far I only saw the requirement in Jaeger. I'd be happen to move this to common utility if it is used in other places.

[lalitb]

how about using run-time determination if it's not set by compiler, something like:

    const int value { 0x01 };
    bool isLittleEndian = ( *(static_cast<const unsigned char *>(static_cast<const void *>(&value))) == 0x01);

[ThomsonTan]

I'd like to avoid the runtime check as it is a static property known at compile time. I think our current definition should cover most real cases if not all.

[bogdandrutu]

For a reference implementation see https://github.com/abseil/abseil-cpp/blob/master/absl/base/internal/endian.h I think you can use that code (not directly if avoiding dependency is needed) but is a good implementation.

[lalitb]

LGTM.

[codecov]

Codecov Report

Merging #832 (dfe421b) into main (5acf4cf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #832   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         156      156           
  Lines        6614     6614           
=======================================
  Hits         6316     6316           
  Misses        298      298           

[maxgolov]

I'm Okay with the essence of this change. But I think it would have been nicer to refactor the implementation into separate utility header. This refactor won't anyhow degrade the perf, if you make the function static inline. By the looks of it what you are doing is htonllll , which should really be standard utility function. I believe we'd need this in other exporters.