C89 API Proposal #1

isaachier · 2018-03-06T15:49:38Z

Meant to be compatible with C++, so pure ANSI C.

rnburn · 2018-03-06T16:13:37Z

How do the tracer and span objects get destroyed?

isaachier · 2018-03-06T16:15:45Z

The spans and tracer need to be derived from the destructible interface. Not sure what I was doing here. Will fix.

rnburn · 2018-03-06T16:19:10Z

src/opentracing-c/tracer.h

+     */
+    int (*inject)(struct opentracing_tracer* tracer,
+                  opentracing_propagation_format format,
+                  void* carrier);


where's the extract function?

Another oversight. Thanks.

isaachier · 2018-03-06T16:16:36Z

.clang-format

+    Priority:        1
+  - Regex:           '^"'
+    Priority:        2
+  - Regex:           '^"jaegertracingc/'


Need to change to opentracingc.

isaachier · 2018-03-06T16:17:17Z

README.md

Preserve original title. Make this subtext.

isaachier · 2018-03-06T16:18:46Z

src/opentracing-c/tracer.h

+} opentracing_start_span_options;
+
+/** Tracer interface. */
+typedef struct opentracing_tracer {


Need to extend destructible.

isaachier · 2018-03-06T16:19:29Z

src/opentracing-c/value.h

+    /** Value type. */
+    opentracing_value_type type;
+    /** Value storage. */
+    union {


Avoid anonymous union.

isaachier · 2018-03-06T16:20:37Z

src/opentracing-c/span.h

+ * span ID, baggage).
+ * @extends opentracing_destructible
+ */
+typedef struct opentracing_span_context {


Needs to extend destructible.

isaachier · 2018-03-06T16:21:03Z

src/opentracing-c/span.h

+struct opentracing_tracer;
+
+/** Span interface. */
+typedef struct opentracing_span {


Needs to extend destructible.

rnburn · 2018-03-06T16:23:50Z

Could we include a dynamic loading interface as part of the C API?

I think an app linking to the C-API but being able to dynamically load a C++ tracer with a C-C++ bridge would be a good scenario to support.

isaachier · 2018-03-06T16:27:35Z

Definitely. I'll work on that too unless you want to do it. Another eventual goal would be to implement a bridge on top of the C++ implementation for C libraries looking to adopt a C++ client without wrapping it themselves.

rnburn · 2018-03-06T23:06:57Z

src/opentracing-c/tracer.c

+    (void) tracer;
+    (void) operation_name;
+    (void) options;
+    null_span* span = (null_span*) malloc(sizeof(null_span));


Does the noop tracer even need to allocate memory? Couldn't it just return a pointer to a single instance of null_span?

In this case yes, and I can change it, as long as that wouldn't be the case for a real tracer.

I was tempted to make advertise this API as not allocating any heap memory, but that wouldn't be true for any instrumented process using a tracer. The only way around that is essentially capping the size of each object and reserving space for it in a stack buffer. See sockaddr_storage details here for a similar idea with IPv4 and IPv6 addresses.

rnburn · 2018-03-06T23:09:48Z

src/opentracing-c/tracer.c

+    opentracing_span base;
+    opentracing_span_context null_span_context;
+    opentracing_tracer* null_tracer;
+} null_span;


Any reason you're using the name null instead of noop which is what it's called in the other APIs (e.g. go, java)?

Really? I don't have a strong preference, but I find no-op hard to spell in code (is it NoOp or Noop?). Null has easy breaking and brings the null object pattern to mind.

That might be arguments for null if this API was considered in insolation; but given that all the other tracing APIs use noop, I think it's going to be confusing to have the C-API use a different naming scheme.

isaachier · 2018-03-07T15:25:41Z

About the dynamic linking @rnburn, I've taken a look at the C++ version which requires a C++ TracerFactory object be loaded from a shared library. That can't be done in a pure C library. What equivalent would you suggest?

rnburn · 2018-03-07T16:34:38Z

Why not use your C inheritance model and return a pointer to some kind of tracer factory struct?

typedef struct opentracing_tracer_factory {
  opentracing_destructible base;
  int make_tracer(struct opentracing_tracer_factorty* factory, const char* config, opentracing_tracer** tracer_out, char** error_message);
};

I used a factory instead of creating a tracer directly to handle the case where you might want to reconstruct a new tracer with a different configuration.

isaachier · 2018-03-07T16:46:48Z

That makes sense. Would you mind if I used a function instead? Like int opentracing_make_tracer(const char* config, opentracing_tracer* tracer); defined in the vendor library?

rnburn · 2018-03-07T16:51:09Z

Yes, I think that would work.

One advantage of the struct is that it could allow you to support having different functions to construct a tracer, but if there's only one I think a function pointer works fine.

rnburn · 2018-03-07T21:16:08Z

src/opentracing-c/span.h

+     */
+    opentracing_bool (*set_tag)(struct opentracing_span* span,
+                                const char* key,
+                                const opentracing_value* value);


Do we want to be returning an error code for these functions? It doesn't seem like there's anything a user would want to do other than log, which a tracer implementation would be doing anyways (the Go API doesn't use error codes).

I guess I agree we can avoid burdening the user with these issues. If you don't think there is a case where a user needs to know that set_tag etc. succeeded or OOM occurred, then I don't have a problem using void as the return type.

rnburn · 2018-03-07T21:21:04Z

src/opentracing-c/span.h

+     *         empty string.
+     */
+    const char* (*baggage_item)(const struct opentracing_span* span,
+                                const char* key);


Does this return an allocated string that must be freed?

If not, what happens if a user gets a baggage item, then calls set_baggage_item to set a new value for the key?

That is a fair point, but I guess it relies heavily on the implementation of this map (which I have left undefined). My gut tells me this should be a case of invalidated pointer, similar to getting an iterator from std::vector, pushing an item to the back of the vector, and attempting to dereference the initial iterator. There are no guarantees we won't need to reallocate the underlying buffer with that push, so all references are invalidated. If you are OK with that, I will add it to the docs.

isaachier · 2018-03-16T16:47:30Z

I guess so. I can change it to a void pointer, but I realized while writing this that the magic number would only work with ABI-compatible types. So casting from an old type definition to a new type definition would be a problem even within the same vendor (not sure how this would happen, maybe with dynamic loading). So I think I should just make a method like validate or something that could take a vendor with version number as string arguments.

isaachier · 2018-03-16T19:27:45Z

Rethinking this yet again... A void pointer actually means nothing if we can't cast it to "check" for properties. I'm back to the magic number as the simplest solution. Consider this problem:

typedef struct base_type {
    const void* opaque_handle;
} base_type;

typedef struct derived_type_a {
    base_type base;
} derived_type_a;

static const char* type_a_id = "type a";

static int check_type_a(const base_type* base)
{
    return strcmp((const char*) base->opaque_handle, type_a_id) == 0;
}

static const int type_b_id = 2;

typedef struct derived_type_b {
    base_type base;
} derived_type_b;

static int check_type_b(const base_type* base)
{
    return type_b_info == *(const int*) base->opaque_handle;
}

int main()
{
    derived_type_a a;
    ((base_type*) &a)->opaque_handle = type_a_id;

    derived_type_b b;
    ((base_type*) &b)->opaque_handle = &type_b_id;

    base_type* base = &b;
    check_type_a(b);  /* Potential segfault */

    return 0;
}

isaachier · 2018-03-16T19:30:12Z

An interesting C++ question related to implementing RTTI: https://stackoverflow.com/q/7525942/1930331.

rnburn · 2018-03-16T19:38:25Z

This is the way I would see it working:

typedef struct dummy_type {
} dummy;

typedef struct base_type {
    const void* type_signature;
} base_type;

typedef struct derived_type_a {
    base_type base;
    int a;
} derived_type_a;

static dummy type_a_id;

static int check_type_a(const base_type* base)
{
    return  base->type_signature == &type_a_id;
}

static dummy type_b_id;

typedef struct derived_type_b {
    base_type base;
    double b;
} derived_type_b;

static int check_type_b(const base_type* base)
{
    return  base->type_signature == &type_b_id;
}

int main()
{
    derived_type_a a;
    ((base_type*) &a)->type_signature = &type_a_id;

    derived_type_b b;
    ((base_type*) &b)->type_signature = &type_b_id;

    base_type* base = &b;
    check_type_a(b);

    return 0;
}

isaachier · 2018-03-16T22:44:46Z

OK so that is exactly what I saw in the StackOverflow question. I think it works here, seeing as we have no other RTTI. The only thing I saw there was that the address is unique as long as the module is loaded. Not 100% sure how that could affect this.

isaachier · 2018-03-19T12:28:30Z

@rnburn are you okay the new proposal for a const void* along with length int so the caller can at least avoid accessing beyond the boundaries of the underlying type?

rnburn · 2018-03-21T15:17:21Z

src/opentracing-c/tracer.h

+        opentracing_propagation_format format,
+        void* carrier,
+        opentracing_span_context** span_context) OPENTRACINGC_NONNULL();
+} opentracing_tracer;


Could we add a close function for the tracer?

Well the question is whether or not destroy is implicitly the same as close. I guess not. Good idea.

rnburn · 2018-03-21T15:21:24Z

So not something that would affect the API at all, but would you consider following a directory structure where the installed headers are separated from the src directory and non-installed headers. IMO it makes it easer to browse the API since you only need to look at the include directory.

This is the directory structure used by all of the boost libraries (see for example boost/filesystem).

rnburn · 2018-03-21T15:23:27Z

Hey @heyleke -- any thoughts on the C API?

It allows support for tracers completely in C and doesn't require C11, which is what I think you were originally interested in.

isaachier · 2018-03-21T15:30:28Z

I get that. OTOH I find the use of an include directory a nuisance. There are projects with both styles.

isaachier · 2018-03-29T13:40:07Z

@rnburn are you ready to accept? I know it would be better if others reviewed, but just your personal review?

rnburn · 2018-03-29T14:14:42Z

@isaachier - the part I don't like is renaming the noop tracer to the null tracer, as noop is consistently used across all the other OT APIs.

But if there's others that buy into moving towards null instead of noop for the APIs, then I'd be ok with it.

isaachier · 2018-03-29T14:42:14Z

If that is the only thing, I'm fine with changing it.

isaachier · 2018-03-29T14:45:05Z

Done

rnburn · 2018-03-29T14:54:41Z

src/opentracing-c/tracer.h

+        struct opentracing_tracer* tracer,
+        opentracing_propagation_format format,
+        void* carrier,
+        const opentracing_span_context* span_context) OPENTRACINGC_NONNULL_ALL;


There was some discussion on opentracing/opentracing-go#127 on getting rid of the format parameter, which is why it's not in the C++ API. Does it make sense to have it for the C API?

rnburn · 2018-03-29T23:20:37Z

This looks good to me.

The only question I had was whether we should be using the propagation_format argument since the consensus from opentracing/opentracing-go#127 seemed to be that we should drop them. But I'll let you decide whether that's a good idea for OT-C or not.

isaachier · 2018-03-30T12:20:42Z

Thanks for approving. I see what you are saying. Maybe I can just use an opaque carrier instead of a format enum.

isaachier · 2018-03-30T15:31:04Z

@rnburn I updated the methods to account for all cases of the enum. Please let me know what you think when you have a chance.

Import initial work

a6a0711

rnburn reviewed Mar 6, 2018

View reviewed changes

isaachier commented Mar 6, 2018

View reviewed changes

Isaac Hier added 5 commits March 6, 2018 13:18

Some fixes and implement null tracer

fbf4867

Fix anonymous union

391231f

Improve test case, build static lib

b825a09

Fix GitHub token

cb8cf05

Fix invalid allocation of null span context

e333c13

isaachier force-pushed the master branch from 56b69df to e333c13 Compare March 6, 2018 22:23

Isaac Hier added 2 commits March 6, 2018 17:26

Fix GitHub token again

9c5a513

Move toolchain before project command

d504426

rnburn reviewed Mar 6, 2018

View reviewed changes

isaachier added 3 commits March 7, 2018 07:39

Use timespec where available and reimplement if not

c21bbfe

Work on exporting symbols, remove uses of dynamic memory

26a4ef7

Change TODO to note

9dce165

isaachier added 2 commits March 7, 2018 10:52

Add clang-tidy support and apply fix

d29b9c6

Add version header and CMake install

13402df

Include config at the top of each header

b803d53

Add Doxygen link to README along with badges

b60b3b9

rnburn reviewed Mar 7, 2018

View reviewed changes

Add magic number

6f85948

Use length field along with void pointer instead of magic number

1207a41

isaachier mentioned this pull request Mar 19, 2018

Create README.md #2

Closed

rnburn reviewed Mar 21, 2018

View reviewed changes

isaachier added 2 commits March 21, 2018 11:40

Use pedantic errors

0bc0f80

Add close method to tracer

002bb7c

Change null to noop

908b164

rnburn reviewed Mar 29, 2018

View reviewed changes

rnburn approved these changes Mar 29, 2018

View reviewed changes

Isaac Hier added 4 commits March 30, 2018 05:27

Remove format enum

1cd217b

Remove format from README

45b58f0

Add methods for each propagation type

a86b022

Update README

c584321

isaachier merged commit 02bc878 into opentracing:master Apr 2, 2018

C89 API Proposal #1

C89 API Proposal #1

Conversation

isaachier commented Mar 6, 2018

rnburn commented Mar 6, 2018

isaachier commented Mar 6, 2018

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

rnburn commented Mar 6, 2018

isaachier commented Mar 6, 2018

Choose a reason for hiding this comment

isaachier Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

rnburn Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Mar 7, 2018

rnburn commented Mar 7, 2018

isaachier commented Mar 7, 2018

rnburn commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

isaachier commented Mar 16, 2018 • edited Loading

isaachier commented Mar 16, 2018 • edited Loading

isaachier commented Mar 16, 2018

rnburn commented Mar 16, 2018 • edited Loading

isaachier commented Mar 16, 2018

isaachier commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn commented Mar 21, 2018 • edited Loading

rnburn commented Mar 21, 2018

isaachier commented Mar 21, 2018 • edited Loading

isaachier commented Mar 29, 2018

rnburn commented Mar 29, 2018

isaachier commented Mar 29, 2018

isaachier commented Mar 29, 2018

Choose a reason for hiding this comment

rnburn commented Mar 29, 2018

isaachier commented Mar 30, 2018

isaachier commented Mar 30, 2018

isaachier Mar 7, 2018 •

edited

Loading

rnburn Mar 6, 2018 •

edited

Loading

isaachier Mar 7, 2018 •

edited

Loading

isaachier commented Mar 16, 2018 •

edited

Loading

isaachier commented Mar 16, 2018 •

edited

Loading

rnburn commented Mar 16, 2018 •

edited

Loading

rnburn commented Mar 21, 2018 •

edited

Loading

isaachier commented Mar 21, 2018 •

edited

Loading