Add SpanId class.

[g-easy]

No description provided.

[reyang]

Curious:

1. Do we need this to be public?
2. Which naming style are we following?

[g-easy]

IMO it doesn't have to be public, but my reasoning is that span_id is a rigidly defined part of the otel data model and having a size constant is useful.

There would be e.g. SpanId::kSize and also TraceId::kSize

[maxgolov]

Re. file naming convention - if the only class in that module is SpanId , should we name the file SpanId[.cpp|.hpp] or SpanId[.cc|.hh] ? .cpp looks a bit better to my eye, others may have a different opinion :)

[g-easy]

I think we should keep it as-is (api/include/opentelemetry/trace/span_id.h) to match e.g. api/include/opentelemetry/nostd/string_view.h

[rnburn]

Any reason we're using an array of 8-bit numbers to represent this type instead of a uint64_t?

A char array like this will have a byte-level alignment (e.g. alignof(SpanId) == 1)) and will be less performant for doing basic operations like equality check, assignment, etc.

[g-easy]

I wanted to avoid implying that this is an integer, and thereby avoid endianness issues. SpanId is an opaque 8-byte binary blob.

[jmacd]

This has been discussed in the protocol repository, fwiw: open-telemetry/opentelemetry-proto#52

(It looks like we'll end up on using a byte array in the protocol, which doesn't preclude using 64-bit integers here.)

[g-easy]

Can we defer this as an implementation detail, or do we treat alignment as part of the ABI, and therefore have to set it in stone?

[jmacd]

This has been discussed in the protocol repository, fwiw: open-telemetry/opentelemetry-proto#52

(It looks like we'll end up on using a byte array in the protocol, which doesn't preclude using 64-bit integers here.)

[g-easy]

Folks, I've respun this PR on top of #13 which added the start of the bazel build system.

This has the start of a span_id class, and an example program that shows how to consume the //api target. No work on //sdk yet.

Please take a fresh look at the changes, especially the BUILD parts and the directory structures.

[g-easy]

Someone please remind me: do we need the license comment on basically every file?

[g-easy]

I'm not comfortable with non-trivial implementations in headers. Is this okay?

I don't know what the alternative is, if we stick to the requirement that the api is strictly header-only.

opentelemetry::trace::SpanId* id = GetGlobalTracer::MakeSpanId(args...); seems a bit extreme.

Can we make basic parts of the data model (eg SpanId, TraceId, Attribute) have concrete implementations in the API that can't be overridden by e.g. a dynamically loaded tracer at runtime.

[g-easy]

Friendly ping.

[g-easy]

FYI:

void f(const opentelemetry::trace::SpanId& span, opentelemetry::nostd::span<uint8_t, 8> out) {
  span.CopyBytesTo(out);
}

bazel build -c opt --save_temps

_Z1fRKN13opentelemetry5trace6SpanIdENS_5nostd4spanIhLm8EEE:
        movq    (%rdi), %rax
        movq    %rax, (%rsi)
        ret

[rnburn]

If you pass SpanId by value does it still use a register?

[g-easy]

No:

_Z1gN13opentelemetry5trace6SpanIdENS_5nostd4spanIhLm8EEE:
        movq    %rdi, (%rsi)
        ret

Maybe we should add a comment encouraging users to pass span ids as values.

[rnburn]

If you keep the same approach but use an int64 (you can still cast it back to a char[]), it should pass in a register like a regular integer argument.

[g-easy]

Sorry, I miscommunicated. When passing spanid by value, uint8_t rep_[kSize] does become a register. I think the optimization you're going for does already happen.

[rnburn]

Ok, so %rdi is storing the TraceId value and %rsi is storing the pointer from the span -- yeah that looks good.

[g-easy]

This is my understanding, yes. :)

[g-easy]

Also in #10 (comment) I was trying to show that the memcpy compiles to two MOV instructions on Intel, as discussed during the weekly meeting.