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.

Sign up for GitHub

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

Fix endianness of Jaeger IDs for transmission #832

Merged
merged 4 commits into from
Jun 10, 2021
Merged

Fix endianness of Jaeger IDs for transmission #832

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1923f05
Fix endianess of Jaeger IDs for transmission
ThomsonTan Jun 7, 2021
2965d0b
Fix naming conflict
ThomsonTan Jun 7, 2021
b89ca2c
Merge branch 'main' into FixJaegerIds
lalitb Jun 8, 2021
dfe421b
Merge branch 'main' into FixJaegerIds
lalitb Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions exporters/jaeger/include/opentelemetry/exporters/jaeger/recordable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,46 @@
#include <opentelemetry/sdk/trace/recordable.h>
#include <opentelemetry/version.h>

#if (defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && \
__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
# define JAEGER_IS_LITTLE_ENDIAN 1
#elif defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && \
__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
# define JAEGER_IS_LITTLE_ENDIAN 0
#elif defined(_WIN32)
# define JAEGER_IS_LITTLE_ENDIAN 1
#else
# error "Endian detection needs to be set up for your compiler"
Copy link
Member

@lalitb lalitb Jun 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

#endif

OPENTELEMETRY_BEGIN_NAMESPACE
namespace exporter
{
namespace jaeger
{

#if JAEGER_IS_LITTLE_ENDIAN == 1

# if defined(__clang__) || \
(defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ >= 8) || __GNUC__ >= 5))
inline uint64_t bswap_64(uint64_t host_int)
{
return __builtin_bswap64(host_int);
}

# elif defined(_MSC_VER)
inline uint64_t bswap_64(uint64_t host_int)
{
return _byteswap_uint64(host_int);
}

# else
# error "Port need to support endianess conversion"

# endif

#endif

using namespace jaegertracing;

class Recordable final : public sdk::trace::Recordable
Expand Down
13 changes: 13 additions & 0 deletions exporters/jaeger/src/recordable.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,25 @@ void Recordable::PopulateAttribute(nostd::string_view key, const common::Attribu
void Recordable::SetIdentity(const trace::SpanContext &span_context,
trace::SpanId parent_span_id) noexcept
{
// IDs should be converted to big endian before transmission.
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/jaeger.md#ids
#if JAEGER_IS_LITTLE_ENDIAN == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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))

Copy link
Member

@lalitb lalitb Jun 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

span_->__set_traceIdHigh(
bswap_64(*(reinterpret_cast<const int64_t *>(span_context.trace_id().Id().data()))));
span_->__set_traceIdLow(
bswap_64(*(reinterpret_cast<const int64_t *>(span_context.trace_id().Id().data()) + 1)));
span_->__set_spanId(
bswap_64(*(reinterpret_cast<const int64_t *>(span_context.span_id().Id().data()))));
span_->__set_parentSpanId(
bswap_64(*(reinterpret_cast<const int64_t *>(parent_span_id.Id().data()))));
#else
span_->__set_traceIdLow(
*(reinterpret_cast<const int64_t *>(span_context.trace_id().Id().data())));
span_->__set_traceIdHigh(
*(reinterpret_cast<const int64_t *>(span_context.trace_id().Id().data()) + 1));
span_->__set_spanId(*(reinterpret_cast<const int64_t *>(span_context.span_id().Id().data())));
span_->__set_parentSpanId(*(reinterpret_cast<const int64_t *>(parent_span_id.Id().data())));
#endif

// TODO: set trace_state.
}
Expand Down
8 changes: 8 additions & 0 deletions exporters/jaeger/test/jaeger_recordable_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace nostd = opentelemetry::nostd;
namespace sdktrace = opentelemetry::sdk::trace;

using namespace jaegertracing;
using namespace opentelemetry::exporter::jaeger;

TEST(JaegerSpanRecordable, SetIdentity)
{
Expand All @@ -39,10 +40,17 @@ TEST(JaegerSpanRecordable, SetIdentity)

std::unique_ptr<thrift::Span> span{rec.Span()};

#if JAEGER_IS_LITTLE_ENDIAN == 1
EXPECT_EQ(span->traceIdLow, opentelemetry::exporter::jaeger::bswap_64(trace_id_val[1]));
EXPECT_EQ(span->traceIdHigh, opentelemetry::exporter::jaeger::bswap_64(trace_id_val[0]));
EXPECT_EQ(span->spanId, opentelemetry::exporter::jaeger::bswap_64(span_id_val));
EXPECT_EQ(span->parentSpanId, opentelemetry::exporter::jaeger::bswap_64(parent_span_id_val));
#else
EXPECT_EQ(span->traceIdLow, trace_id_val[0]);
EXPECT_EQ(span->traceIdHigh, trace_id_val[1]);
EXPECT_EQ(span->spanId, span_id_val);
EXPECT_EQ(span->parentSpanId, parent_span_id_val);
#endif
}

TEST(JaegerSpanRecordable, SetName)
Expand Down