[WIP] W3C tracecontext #9

reyang · 2019-10-18T03:23:49Z

Trying to get initial feedback (I know there will be a LOT) before I continue on the implementation.
Feel free to use this initial PR to discuss whatever topic you want - styling, naming, C or C++, macro, convention..

g-easy · 2019-10-18T03:30:31Z

In my reading of https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/library-layout.md, W3C tracecontext is trace/propagation/ rather than context/ (the latter is for in-process context propagation, the former is between processes)

reyang · 2019-10-18T03:38:31Z

In my reading of https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/library-layout.md, W3C tracecontext is trace/propagation/ rather than context/ (the latter is for in-process context propagation, the former is between processes)

Hmm, I think the data structure is part of trace, while the formatters (e.g. the HTTP string format) is part of context/propagation?

In Python we had similar discussion, and we ended up with:

TraceParent and TraceState classes are defined in trace
HTTP string format parsing/serialization are implemented in context.

Happy to hear different ideas.

reyang · 2019-10-18T03:43:44Z

opentelemetry_api/opentelemetry/context/traceparent.h

+#pragma pack(pop)
+
+/* unchecked version */
+status_t _trace_parent_from_string(TraceParent* trace_parent, const char* s);


The unchecked version doesn't validate input (e.g. nullptr), which gives perf benefit when used internally (e.g. the caller already validated the input).
Let me know if you think this is weird. (I probably learned this style from driver/kernel development).

reyang · 2019-10-18T03:45:37Z

opentelemetry_api/opentelemetry/context/traceparent.c

+status_t _trace_parent_from_string(TraceParent* trace_parent, const char* s)
+{
+  unsigned int value;
+  /* if this function failed, trace_parent could be polluted,


We should establish the convention across APIs.

reyang · 2019-10-18T03:49:20Z

opentelemetry_api/opentelemetry/context/traceparent.c

+  return STATUS_ERROR_UNKNOWN;
+}
+
+status_t trace_parent_from_string(TraceParent* trace_parent, const char* s)


Personally I'm in favor of PascalCase instead of snake_case. Should we vote? 😄

I think we should base these functions off of a back-ported string_view instead of using null terminated strings (i.e. we copy an implementation of string_view that's compatible with c++17's string_view; we can even opt in to std::string_view like absl if they're using c++17+).

Lots of systems don't use null terminated strings (for example nginx, lua)

So having the api require a null-terminated c-style string would require users to make a copy before calling the function.

Personally I'm in favor of PascalCase instead of snake_case

Instead of debating convention on a case-by-case basis, could we use #5 to adopt one of the standard naming conventions globally and then redo the code to match that.

I'm very much in favor of string_view.

Also in favor of PascalCase, we can discuss that in #5 if it's more appropriate.

reyang · 2019-10-18T03:50:29Z

opentelemetry_api/opentelemetry/context/traceparent.h

+extern "C" {
+#endif
+
+#pragma pack(push, 1)


This might not work with some compilers.

I think we shouldn't rely on pragma pack.

This shouldn't be an issue since no-one should be serializing data by memcpying a struct.

What's purpose of packing TraceParent struct?

g-easy · 2019-10-18T04:20:42Z

Hmm, I think the data structure is part of trace, while the formatters (e.g. the HTTP string format) is part of context/propagation?

I think the TraceContext data structure belongs in trace/ and the formatters in trace/propagation/

context/ would have a TLS data structure which contains a TraceContext and a TagsContext (names are up for discussion)

rnburn · 2019-10-21T23:16:38Z

opentelemetry_api/opentelemetry/status.h

+
+#include <stdint.h>
+
+typedef int32_t status_t;


If we're going to use error codes, I'd rather use the standard library's std::error_code.

It's somewhat of a pain to work with, but IMO it's still better than rolling your own

rnburn · 2019-10-21T23:24:27Z

opentelemetry_api/opentelemetry/status.h

+#ifndef OPENTELEMETRY_STATUS_H_
+#define OPENTELEMETRY_STATUS_H_
+
+#include <stdint.h>


I think s/#include <stdint.h>/#include <cstdint>/ and have the c api as a separate project without dual purpose headers.

maxgolov · 2019-10-21T23:38:15Z

opentelemetry_api/opentelemetry/context/traceparent.c

+
+#include "traceparent.h"
+
+status_t _uint_from_hex_string(unsigned int* value, const char* s) {


Technically "status_t" not a POSIX standard type..
While anything that has _t in its type name is reserved for POSIX.
Cit.: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
See section about the "Suffix" -- _t reserved for ANY HEADER..

I think many good open source libraries don't necessarily follow that rule, e.g.

nginx widely uses _t suffixes.

Android Framework uses status_t suffix for a non-POSIX type.. In fact ours would then clash with Android's definition!

Windows SDK uses _t suffix for C-types in several SDK headers..

But the best practice would probably either avoid using t suffix, or ensure that all C-API types have a unique prefix, such as otel or at least ot

I vote for not using *_t suffix on our datatypes. I'd prefer opentelemetry::Status.

maxgolov · 2019-10-21T23:42:43Z

opentelemetry_api/opentelemetry/context/traceparent.h

+
+/* unchecked version */
+status_t _trace_parent_from_string(TraceParent* trace_parent, const char* s);
+status_t _trace_parent_to_string(const TraceParent* trace_parent, char* s);


My understanding is that function names starting with underscore are reserved (by naming convention) for "standard library" functions.

g-easy · 2019-10-22T00:28:01Z

opentelemetry_api/opentelemetry/context/traceparent.c

@@ -0,0 +1,106 @@
+/* Copyright 2019, OpenTelemetry Authors


I would like the implementation to be in C++, not C.

I agree. This is C++ SIG and implementations should preferably be in C++.

g-easy · 2019-10-22T00:28:40Z

opentelemetry_api/opentelemetry/context/traceparent.c

+
+#include "traceparent.h"
+
+status_t _uint_from_hex_string(unsigned int* value, const char* s) {


I vote for not using *_t suffix on our datatypes. I'd prefer opentelemetry::Status.

g-easy · 2019-10-22T00:29:31Z

opentelemetry_api/opentelemetry/context/traceparent.c

+  return STATUS_ERROR_UNKNOWN;
+}
+
+status_t trace_parent_from_string(TraceParent* trace_parent, const char* s)


I'm very much in favor of string_view.

Also in favor of PascalCase, we can discuss that in #5 if it's more appropriate.

g-easy · 2019-10-22T00:29:57Z

opentelemetry_api/opentelemetry/context/traceparent.h

+
+#include "../status.h"
+
+#ifdef __cplusplus


I would like the headers to be C++. The C wrapper should live somewhere else.

Re. headers - there is no official Cpp Core Guideline on how to name the headers (some people even prefer .h for c++ code). But I personally find it easier to navigate thru source code directory when the filename is telling what compiler it's gonna be compiled with: like .c|.h for C, .cpp|.hpp for C++ (or .cc|.hh, or .cxx|.hxx, depending on one's personal preference). I agree that the headers must be c++ by default, with an optional pure C projection of some sort. And the header filename should explicitly express what language it is for..

g-easy · 2019-10-22T00:30:49Z

opentelemetry_api/opentelemetry/context/traceparent.h

+   limitations under the License.
+*/
+
+#ifndef OPENTELEMETRY_CONTEXT_TRACEPARENT_H_


(this came up in the meeting) At least Ryan and I are fans of #pragma once

Do we need to support older compilers where we need to use the header guard? If so, which ones?

I've never seen a compiler that doesn't support it. Looking at the portability section of https://en.wikipedia.org/wiki/Pragma_once even old Sun and IBM compilers work with it.

I'd say adopt it -- you can always change to include guards if someone complains.

I vote for the pragma then. :)

g-easy · 2019-10-22T00:31:44Z

opentelemetry_api/opentelemetry/context/traceparent.h

+extern "C" {
+#endif
+
+#pragma pack(push, 1)


I think we shouldn't rely on pragma pack.

This shouldn't be an issue since no-one should be serializing data by memcpying a struct.

g-easy · 2019-10-22T00:33:01Z

opentelemetry_api/opentelemetry/context/traceparent.h

+  uint8_t trace_id[16];
+  uint8_t parent_id[8];
+  uint8_t trace_flags;
+} TraceParent;


I think this should be a class SpanContext and there should be a helper function like:

SpanContext FromTraceParentHeader(string_view header);

g-easy · 2019-10-22T00:33:41Z

opentelemetry_api/opentelemetry/context/traceparent.h

+status_t _trace_parent_to_string(const TraceParent* trace_parent, char* s);
+
+/* checked version */
+status_t trace_parent_from_string(TraceParent* trace_parent, const char* s);


I would suggest keeping the public API small and only having the checked version.

Makes sense. All the public API should validate input.

tigrannajaryan · 2019-10-25T13:51:17Z

opentelemetry_api/opentelemetry/context/traceparent.c

+
+#include "traceparent.h"
+
+status_t _uint_from_hex_string(unsigned int* value, const char* s) {


Pointers to strings without lengths are IMO problematic. If you don't want to incur a runtime cost in the release version at the minimum I would assert the length and !nullptr for s.

However instead of char* I think we should be using C++ and either std::string or std::string_view (char* should be a justified exception).

Pointers to strings without lengths are IMO problematic. If you don't want to incur a runtime cost in the release version at the minimum I would assert the length and !nullptr for s.

Length check is covered here (by checking if c is not '\0').
nullptr check is not covered since this function is considered as unchecked (not public), it is intended to be called only by the OpenTelemetry C++ API/SDK code, which has already done the nullptr check.

However instead of char* I think we should be using C++ and either std::string or std::string_view (char* should be a justified exception).

std::string requires memory allocation if the input is a plain buffer, I was trying to avoid memory allocation here.
std::string_view requires very recent version of C++ 17.

Use a back-ported string_view as I was suggesting here

It keeps the same interface as std::string_view but doesn't require c++17. That's what we did with the opentracing-cpp api

You can even a prepreocessor switch to opt into std::string_view if you're using c++17 (what absl does)

tigrannajaryan · 2019-10-25T13:56:09Z

opentelemetry_api/opentelemetry/context/traceparent.c

+  {
+    result += c - '0';
+  }
+  else if (c >= 'a' && c <= 'f')


What about uppercase hex digits?

I'd make hexdigit2int a function and let compiler inline it for me.

What about uppercase hex digits?

Traceparent only allows lower case hex digits: https://www.w3.org/TR/trace-context/#traceparent-header-field-values

I'd make hexdigit2int a function and let compiler inline it for me.

Good idea. I was originally trying to see if we want to target C89 (which doesn't have inline).
If we're moving to pure C++, will use inline.

tigrannajaryan · 2019-10-25T14:09:31Z

opentelemetry_api/opentelemetry/context/traceparent.h

+extern "C" {
+#endif
+
+#pragma pack(push, 1)


What's purpose of packing TraceParent struct?

tigrannajaryan · 2019-10-25T14:12:24Z

opentelemetry_api/opentelemetry/context/traceparent.h

+status_t trace_parent_from_string(TraceParent* trace_parent, const char* s);
+status_t trace_parent_to_string(const TraceParent* trace_parent, char* s);
+
+#ifdef __cplusplus


Wasn't there a decision that this is going to be a C++ library and a C library can be implemented later which will wrap this one?
Is it a stated goal that the headers must compile and be usable both for C++ and C?

It was unclear at the moment I was preparing the PR.
Now we've discussed and agreed that we should have C++ only. C support should be a separate wrapper (which can be hosted in this project for now, later we might want to move it to a separate repo).

tigrannajaryan · 2019-10-25T14:14:08Z

opentelemetry_api/opentelemetry/context/traceparent.c

+  s += 2;
+  IF_FALSE_RETURN('-' == *s, STATUS_ERROR_INPUT_ILLEGAL)
+  s += 1;
+  for (int i = 0; i < 16; i++)


I'd use sizeof(trace_parent->trace_id)/sizeof(trace_parent->trace_id[0]) instead of 16. I know it is defined in w3c standard but I still prefer code correctness not to depend on some declaration far away to match what I think it should be.

tigrannajaryan · 2019-10-25T14:16:10Z

opentelemetry_api/opentelemetry/context/traceparent.c

+}
+
+status_t trace_parent_from_string(TraceParent* trace_parent, const char* s)
+{


Is this intended to be the public version which checks the input arguments? I don't see length checks for s.

Length check is covered by detecting '\0'.

Origin/propagators

Change OTLP HTTP content_type default to binary (open-telemetry#2558)

W3C tracecontext initial commit

6640262

reyang requested review from g-easy, jmacd, maxgolov, rnburn and tigrannajaryan as code owners October 18, 2019 03:23

reyang commented Oct 18, 2019

View reviewed changes

g-easy requested a review from bogdandrutu October 18, 2019 04:18

rnburn reviewed Oct 21, 2019

View reviewed changes

maxgolov reviewed Oct 21, 2019

View reviewed changes

g-easy reviewed Oct 22, 2019

View reviewed changes

tigrannajaryan reviewed Oct 25, 2019

View reviewed changes

reyang closed this Nov 4, 2019

reyang mentioned this pull request Jan 7, 2020

Adding a tracer provider #24

Merged

reyang deleted the tracecontext branch July 23, 2020 04:35

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

Merge pull request open-telemetry#9 from Tianlin-Zhao/origin/propagators

b09a788

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#9 from open-telemetry/main

b5cb0f7

Change OTLP HTTP content_type default to binary (open-telemetry#2558)


		#include "traceparent.h"

		status_t _uint_from_hex_string(unsigned int* value, const char* s) {

[WIP] W3C tracecontext #9

[WIP] W3C tracecontext #9

Conversation

reyang commented Oct 18, 2019

g-easy commented Oct 18, 2019

reyang commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn Oct 21, 2019 • edited Loading

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 Oct 18, 2019

Choose a reason for hiding this comment

rnburn Oct 21, 2019 • edited by g-easy Loading

Choose a reason for hiding this comment

maxgolov Oct 21, 2019 • edited Loading

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

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

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

tigrannajaryan Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn Oct 21, 2019 •

edited

Loading

rnburn Oct 21, 2019 •

edited by g-easy

Loading

maxgolov Oct 21, 2019 •

edited

Loading

tigrannajaryan Oct 25, 2019 •

edited

Loading