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.

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

[WIP] W3C tracecontext #9

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Empty file.
14 changes: 14 additions & 0 deletions opentelemetry_api/opentelemetry/context/hello.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/* this file will be removed once we have unit test and CI */

#include <stdio.h>

#include "traceparent.h"
#include "tracestate.h"

int main(int argc, char* argv[])
{
TraceParent trace_parent;
status_t retval = trace_parent_from_string(&trace_parent, "00-12345678901234567890123456789012-1234567890123456-01");
printf("Hello, World! %d\n", retval);
return 0;
}
106 changes: 106 additions & 0 deletions opentelemetry_api/opentelemetry/context/traceparent.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/* Copyright 2019, OpenTelemetry Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#include "traceparent.h"

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

@maxgolov maxgolov Oct 21, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

char c = s[0];
unsigned int result;

if (c >= '0' && c <= '9')
{
result = c - '0';
}
else if (c >= 'a' && c <= 'f')
{
result = c - 'a' + 10;
}
else
{
return STATUS_ERROR_INPUT_ILLEGAL;
}
result <<= 4;
c = s[1];
if (c >= '0' && c <= '9')
{
result += c - '0';
}
else if (c >= 'a' && c <= 'f')
Copy link
Member

Choose a reason for hiding this comment

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

  1. What about uppercase hex digits?
  2. I'd make hexdigit2int a function and let compiler inline it for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. What about uppercase hex digits?

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

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

{
result += c - 'a' + 10;
}
else
{
return STATUS_ERROR_INPUT_ILLEGAL;
}
*value = result;

return STATUS_OK;
}

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

Choose a reason for hiding this comment

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

We should establish the convention across APIs.

do we want to take a copy (perf sucks)?
*/
IF_ERROR_RETURN(_uint_from_hex_string(&value, s));
trace_parent->version = value;
s += 2;
IF_FALSE_RETURN('-' == *s, STATUS_ERROR_INPUT_ILLEGAL)
s += 1;
for (int i = 0; i < 16; i++)
Copy link
Member

@tigrannajaryan tigrannajaryan Oct 25, 2019

Choose a reason for hiding this comment

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

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.

{
IF_ERROR_RETURN(_uint_from_hex_string(&value, s));
trace_parent->trace_id[i] = value;
s += 2;
}
IF_FALSE_RETURN('-' == *s, STATUS_ERROR_INPUT_ILLEGAL)
s += 1;
for (int i = 0; i < 8; i++)
{
IF_ERROR_RETURN(_uint_from_hex_string(&value, s));
trace_parent->parent_id[i] = value;
s += 2;
}
IF_FALSE_RETURN('-' == *s, STATUS_ERROR_INPUT_ILLEGAL)
s += 1;
IF_ERROR_RETURN(_uint_from_hex_string(&value, s));
trace_parent->trace_flags = value;
s += 2;
IF_FALSE_RETURN(('-' == *s || ('\0' == *s)), STATUS_ERROR_INPUT_ILLEGAL)
return STATUS_OK;
}

status_t _trace_parent_to_string(const TraceParent* trace_parent, char* s)
{
return STATUS_ERROR_UNKNOWN;
}

status_t trace_parent_from_string(TraceParent* trace_parent, const char* s)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@rnburn rnburn Oct 21, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 very much in favor of string_view.

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

{
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Length check is covered by detecting '\0'.

IF_FALSE_RETURN(trace_parent, STATUS_ERROR_INPUT_NULL_PTR)
IF_FALSE_RETURN(s, STATUS_ERROR_INPUT_NULL_PTR)

return _trace_parent_from_string(trace_parent, s);
}

status_t trace_parent_to_string(const TraceParent* trace_parent, char* s)
{
IF_FALSE_RETURN(trace_parent, STATUS_ERROR_INPUT_NULL_PTR)
IF_FALSE_RETURN(s, STATUS_ERROR_INPUT_NULL_PTR)

return _trace_parent_to_string(trace_parent, s);
}
51 changes: 51 additions & 0 deletions opentelemetry_api/opentelemetry/context/traceparent.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/* Copyright 2019, OpenTelemetry Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#ifndef OPENTELEMETRY_CONTEXT_TRACEPARENT_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

(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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for the pragma then. :)

#define OPENTELEMETRY_CONTEXT_TRACEPARENT_H_

#include "../status.h"

#ifdef __cplusplus
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

namespace opentelemetry {
namespace context {
extern "C" {
#endif

#pragma pack(push, 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

This might not work with some compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

What's purpose of packing TraceParent struct?

typedef struct
{
uint8_t version;
uint8_t trace_id[16];
uint8_t parent_id[8];
uint8_t trace_flags;
} TraceParent;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

SpanContext FromTraceParentHeader(string_view header);

#pragma pack(pop)

/* unchecked version */
status_t _trace_parent_from_string(TraceParent* trace_parent, const char* s);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

status_t _trace_parent_to_string(const TraceParent* trace_parent, char* s);
Copy link
Contributor

Choose a reason for hiding this comment

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

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


/* checked version */
status_t trace_parent_from_string(TraceParent* trace_parent, const char* s);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. All the public API should validate input.

status_t trace_parent_to_string(const TraceParent* trace_parent, char* s);

#ifdef __cplusplus
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

} /* extern "C" */
} /* namespace context */
} /* namespace opentelemetry */
#endif

#endif /* OPENTELEMETRY_CONTEXT_TRACEPARENT_H_ */
16 changes: 16 additions & 0 deletions opentelemetry_api/opentelemetry/context/tracestate.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/* Copyright 2019, OpenTelemetry Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#include "tracestate.h"
33 changes: 33 additions & 0 deletions opentelemetry_api/opentelemetry/context/tracestate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/* Copyright 2019, OpenTelemetry Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#ifndef OPENTELEMETRY_CONTEXT_TRACESTATE_H_
#define OPENTELEMETRY_CONTEXT_TRACESTATE_H_

#include "../status.h"

#ifdef __cplusplus
namespace opentelemetry {
namespace context {
extern "C" {
#endif

#ifdef __cplusplus
} /* extern "C" */
} /* namespace context */
} /* namespace opentelemetry */
#endif

#endif /* OPENTELEMETRY_CONTEXT_TRACESTATE_H_ */
41 changes: 41 additions & 0 deletions opentelemetry_api/opentelemetry/status.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/* Copyright 2019, OpenTelemetry Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#ifndef OPENTELEMETRY_STATUS_H_
#define OPENTELEMETRY_STATUS_H_

#include <stdint.h>
Copy link
Contributor

@rnburn rnburn Oct 21, 2019

Choose a reason for hiding this comment

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

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


typedef int32_t status_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


enum {
STATUS_OK = 0,
STATUS_ERROR_UNKNOWN = 0x00010000,
STATUS_ERROR_INPUT_NULL_PTR = 0x00010001,
STATUS_ERROR_INPUT_ILLEGAL = 0x00010002,
};

#define IF_ERROR_RETURN(expr) { \
status_t status = (expr); \
if (STATUS_OK != status) \
return status; \
}

#define IF_FALSE_RETURN(expr, retval) { \
if (!(expr)) \
return (retval); \
}

#endif /* OPENTELEMETRY_STATUS_H_ */