jenkins test api

jenkins test windows

jenkins test make check

looks good to me, assuming the array sizes are correct

@cbodley This comment made me look at the "jaeger is present" branch and I would like to raise a concern regarding taking in on-wire format definitions from third party headers (TraceId::kSize and SpanId::kSize constants). It doesn't seem robust.

I think it would be better to define two constants in one of our namespaces to the corresponding integers (16 and 8), use these constants in encode and decode methods and employ static_assert under HAVE_JAEGER to ensure that they are equal to TraceId::kSize and SpanId::kSize. This way, if opentelemetry ever changes these, our encoding wouldn't break (or at least we would know about it).

looks good to me, assuming the array sizes are correct

@cbodley This comment made me look at the "jaeger is present" branch and I would like to raise a concern regarding taking in on-wire format definitions from third party headers (TraceId::kSize and SpanId::kSize constants). It doesn't seem robust.

I think it would be better to define two constants in one of our namespaces to the corresponding integers (16 and 8), use these constants in encode and decode methods and employ static_assert under HAVE_JAEGER to ensure that they are equal to TraceId::kSize and SpanId::kSize. This way, if opentelemetry ever changes these, our encoding wouldn't break (or at least we would know about it).

Makes sense. I went with your previous suggestion, so we no longer need to use those constants:

inline void decode(jspan_context& span_ctx, bufferlist::const_iterator& bl) {
  DECODE_START(254, bl)
  // jaeger is missing, consume the buffer but do not decode it.
  DECODE_FINISH(bl);
  span_ctx = jspan_context();
}

I went with your previous suggestion, so we no longer need to use those constants

+1 -- they are still used in the "jaeger is present" branch but that is out of scope of this PR.

jenkins test make check

jenkins test api

jenkins test windows

SpanId::kSize

@cbodley, this makes sense. we should do that regrdless of this PR. we use these constants in the existing code

jenkins test api

I don't see a point in running any of the suites on this PR since we don't seem to have non-jaeger package builds, at least not on main. Merging after running some manual tests covering jaeger-enabled OSDs <-> non-jaeger client and vice versa.

SpanId::kSize

@cbodley, this makes sense. we should do that regrdless of this PR. we use these constants in the existing code

@yuvalif Not sure if you quoted what you meant to quote in that comment. I'm choosing to interpret it as you agreeing with my suggestion to not use constants from opentelemetry headers directly in encoding code ;)