Add SpanId class. #10

g-easy · 2019-10-25T02:57:50Z

No description provided.

WORKSPACE

examples/helloworld/BUILD

api/src/opentelemetry/trace/span_id.cc

api/include/opentelemetry/trace/span_id.h

examples/helloworld/helloworld.cc

reyang · 2019-10-25T03:27:13Z

api/include/opentelemetry/trace/span_id.h

+class SpanId {
+ public:
+  // The size in bytes of the SpanId.
+  static constexpr size_t kSize = 8;


Curious:

Do we need this to be public?

Which naming style are we following?

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

api/src/opentelemetry/trace/span_id.cc

api/include/opentelemetry/trace/span_id.h

api/src/opentelemetry/trace/span_id.cc

maxgolov · 2019-10-25T06:14:39Z

api/src/opentelemetry/trace/span_id.cc

@@ -0,0 +1,29 @@
+// Copyright 2019, OpenTelemetry Authors


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

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 · 2019-10-28T18:37:38Z

api/include/opentelemetry/trace/span_id.h

+  std::string ToHex() const;
+
+ private:
+  uint8_t rep_[kSize];


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.

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

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

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?

api/src/opentelemetry/trace/BUILD

jmacd · 2019-12-09T23:27:35Z

api/include/opentelemetry/trace/span_id.h

+  std::string ToHex() const;
+
+ private:
+  uint8_t rep_[kSize];


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 · 2019-12-19T07:08:27Z

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 · 2019-12-19T07:12:06Z

api/BUILD

@@ -1,3 +1,17 @@
+# Copyright 2019, OpenTelemetry Authors


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

g-easy · 2019-12-19T07:14:39Z

api/include/opentelemetry/trace/span_id.h

+  {
+    constexpr char kHex[] = "0123456789ABCDEF";
+    std::string s(kSize * 2, ' ');
+    for (int i = 0; i < kSize; ++i)


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 · 2020-01-07T05:16:44Z

Friendly ping.

api/include/opentelemetry/trace/span_id.h

g-easy · 2020-01-16T03:20:41Z

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 · 2020-01-16T04:36:13Z

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

g-easy · 2020-01-16T04:37:30Z

No:

_Z1gN13opentelemetry5trace6SpanIdENS_5nostd4spanIhLm8EEE:
        movq    %rdi, (%rsi)
        ret

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

rnburn · 2020-01-16T04:41:03Z

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 · 2020-01-16T04:43:04Z

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 · 2020-01-16T04:46:05Z

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

g-easy · 2020-01-16T05:02:40Z

This is my understanding, yes. :)

g-easy · 2020-01-16T05:22:59Z

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.

…tors Origin/propagators

Merge from upstream

g-easy requested review from jmacd, maxgolov, reyang, rnburn and tigrannajaryan as code owners October 25, 2019 02:57

g-easy commented Oct 25, 2019

View reviewed changes

WORKSPACE Outdated Show resolved Hide resolved

examples/helloworld/BUILD Outdated Show resolved Hide resolved

reyang reviewed Oct 25, 2019

View reviewed changes

api/src/opentelemetry/trace/span_id.cc Outdated Show resolved Hide resolved

reyang reviewed Oct 25, 2019

View reviewed changes

api/include/opentelemetry/trace/span_id.h Outdated Show resolved Hide resolved

reyang reviewed Oct 25, 2019

View reviewed changes

examples/helloworld/helloworld.cc Outdated Show resolved Hide resolved

reyang reviewed Oct 25, 2019

View reviewed changes

api/src/opentelemetry/trace/span_id.cc Outdated Show resolved Hide resolved

maxgolov reviewed Oct 25, 2019

View reviewed changes

api/include/opentelemetry/trace/span_id.h Outdated Show resolved Hide resolved

maxgolov reviewed Oct 25, 2019

View reviewed changes

api/src/opentelemetry/trace/span_id.cc Outdated Show resolved Hide resolved

maxgolov reviewed Oct 25, 2019

View reviewed changes

rnburn reviewed Oct 28, 2019

View reviewed changes

api/src/opentelemetry/trace/BUILD Outdated Show resolved Hide resolved

jmacd approved these changes Dec 9, 2019

View reviewed changes

Initial sketch of span_id, example, and build.

e57940a

g-easy force-pushed the bazel branch from 95bb0dd to e57940a Compare December 19, 2019 07:06

g-easy commented Dec 19, 2019

View reviewed changes

Merge branch 'master' into bazel

74bddc8

rnburn reviewed Jan 7, 2020

View reviewed changes

api/include/opentelemetry/trace/span_id.h Outdated Show resolved Hide resolved

g-easy added 6 commits January 13, 2020 16:48

Merge remote-tracking branch 'origin/master' into bazel

4460619

Use nostd::span.

9d8c63c

Merge remote-tracking branch 'origin/master' into bazel

2c76528

Rename to match Java API.

e3ff806

rm TBD

2e98d79

Add span_id_test.

c826330

g-easy added 3 commits January 15, 2020 16:19

rm examples/, not for this PR.

873cbcf

Restore examples/TBD

a153693

Fix gcc4.8 build.

0ebf537

g-easy changed the title ~~[WIP] Initial sketch of bazel build system.~~ Add SpanId class. Jan 15, 2020

rnburn approved these changes Jan 15, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into bazel

b76e201

g-easy merged commit 5241561 into open-telemetry:master Jan 16, 2020

g-easy deleted the bazel branch January 16, 2020 03:11

0x4b pushed a commit to 0x4b/opentelemetry-cpp that referenced this pull request Jul 31, 2020

Merge pull request open-telemetry#10 from Tianlin-Zhao/origin/propaga…

2f081d8

…tors Origin/propagators

ZDzhao mentioned this pull request Feb 18, 2022

crash in ext::http::client::curl::HttpOperation::Send #1220

Closed

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 21, 2024

Merge pull request open-telemetry#10 from open-telemetry/main

3099d2d

Merge from upstream

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SpanId class. #10

Add SpanId class. #10

g-easy commented Oct 25, 2019

reyang Oct 25, 2019

g-easy Dec 19, 2019

maxgolov Oct 25, 2019

g-easy Jan 15, 2020

rnburn Oct 28, 2019

g-easy Dec 9, 2019

jmacd Dec 9, 2019

g-easy Dec 19, 2019

jmacd Dec 9, 2019

g-easy commented Dec 19, 2019

g-easy Dec 19, 2019

g-easy Dec 19, 2019

g-easy commented Jan 7, 2020

g-easy commented Jan 16, 2020

rnburn commented Jan 16, 2020

g-easy commented Jan 16, 2020

rnburn commented Jan 16, 2020

g-easy commented Jan 16, 2020

rnburn commented Jan 16, 2020

g-easy commented Jan 16, 2020

g-easy commented Jan 16, 2020

Add SpanId class. #10

Add SpanId class. #10

Conversation

g-easy commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-easy commented Dec 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-easy commented Jan 7, 2020

g-easy commented Jan 16, 2020

rnburn commented Jan 16, 2020

g-easy commented Jan 16, 2020

rnburn commented Jan 16, 2020

g-easy commented Jan 16, 2020

rnburn commented Jan 16, 2020

g-easy commented Jan 16, 2020

g-easy commented Jan 16, 2020