Context api alternative implementation by satac2 · Pull Request #180 · open-telemetry/opentelemetry-cpp

satac2 · 2020-07-16T15:46:41Z

Links the context objects together as a linked list where each time new data is written to a context, a new node is added containing that new information and each node has access to all the data before it and none of the data after. Within each node there is another linked list actually containing the key value pairs. This implementation uses no STL and therefore I believe should have no ABI compatibility issues. @reyang @maxgolov.

…d added the contextvars_context.h file

Co-authored-by: Kirk Kelsey <kirkmkelsey@gmail.com>

…o context_api

…-cpp into context_api_content

pyohannes · 2020-07-21T02:06:52Z

+OPENTELEMETRY_BEGIN_NAMESPACE
+namespace context
+{
+using ContextValue = nostd::variant<bool, int64_t, uint64_t, double>;


I think we'll need nostd::shared_ptr<SpanContext> too.

When I tried to add it I got a: variant is implicitly deleted because the default definition would be ill-formed error

So the nostd::shared_ptr does work. But whenever I try to add a unique_ptr of any type, I am getting the implicitly deleted because of ill formed definition error.

nostd::shared_pr has been added is the unique_ptr necessary?

pyohannes · 2020-07-21T02:12:57Z

+  // Creates a context object from a map of keys and identifiers, this will
+  // hold a shared_ptr to the head of the DataList linked list
+  template <class T, nostd::enable_if_t<trace::detail::is_key_value_iterable<T>::value> * = nullptr>
+  Context(const T &keys_and_values)


I'm not 100% sure, maybe I'm missing some part of the high-level view.

I think we'll use Context to obtain the currently active span: we will have a method to obtain the current context, the current context will hold the SpanContext of the active span. I'm trying to figure out necessary steps to make this possible:

When a new span starts, we need to set a new current context. We will be able to get this new context via SetValue("current_span", span_context). Then the new span will be stored in the new current context.

When the span ends, we will need to restore the previous context, as this should be the current context now (returning the parent span as active span).

I think we'll need some kind of parenting of contexts, but I'm not sure if it will be managed by the Context itself or by some other class.

reyang · 2020-07-21T14:51:04Z

+  {
+    Context context = Context(values);
+    nostd::shared_ptr<DataList> last_node;
+    for (nostd::shared_ptr<DataList> data = context.head_; data != nullptr; data = data->next_)


Do we need shared_ptr while iterating through the linked list or there can be an optimization?

Changed to just a ptr.

This can be simplified a bit:

nostd::shared_ptr<DataList>& last = context.head_; while (last->next_ != nullptr) { last = last->next_; } last->next_ = head_;

reyang · 2020-07-21T14:53:49Z

+        }
+      }
+    }
+    return (int64_t)0;


Not blocking this PR, do we need a nil type?

Boost has a boost::blank type for this purpose. The more C++ idiomatic way would be top wrap the return value in a std::optional, but if feel skeptical about extending nostd by this (it only was introduced in C++17).

I also think this shouldn't block this PR.

reyang

LGTM.

…-cpp into context_api_content

reyang · 2020-07-21T19:36:59Z

The change looks good enough to me.

I think we might need some memory testing (e.g. checking if all the allocated linked-list nodes are freed up before exiting the application (e.g. by using our custom malloc/free) to make sure we're actually doing the correct/efficient ref-counting. Probably in another PR.

Looks like there are still open questions/comments, I'll wait until we got approval from @pyohannes.

pyohannes

A few more nits, but I think this is close to being done.

pyohannes · 2020-07-21T18:31:12Z

+        key_        = new char[nostd::string_view(iter->first).size()];
+        key_length_ = nostd::string_view(iter->first).size();
+        memcpy(key_, nostd::string_view(iter->first).data(),
+               nostd::string_view(iter->first).size() * sizeof(char));


Please leave out the explicit nostd::string_view here, and below. It makes the code easier to read.

pyohannes · 2020-07-21T19:12:43Z

+    // and returns that head.
+    DataList(nostd::string_view key, ContextValue value)
+    {
+      key_        = new char[nostd::string_view(key).size()];


No need for the explicit nostd::string_view here, this can just be key.size().

pyohannes · 2020-07-21T19:16:54Z

+    // Builds a data list off of a key and value iterable and returns the head
+    template <class T,
+              nostd::enable_if_t<trace::detail::is_key_value_iterable<T>::value> * = nullptr>
+    DataList(const T &keys_and_vals)


Please add an additional move constructor:

DataList &operator=(DataList &&other) { key_length_ = other.key_length_; value_ = std::move(other.value_); next_ = std::move(other.next_); key_ = other.key_; other.key_ = nullptr; return *this; }

Then you can simplify this constructor and avoid code duplication:

template <class T> DataList(const T &keys_and_vals) : key_{nullptr} { bool first = true; auto *node = this; for (auto &iter : keys_and_vals) { if (first) { *node = std::move(DataList(iter.first, iter.second)); first = false; } else { node->next_ = nostd::shared_ptr<DataList>(new DataList(iter.first, iter.second)); node = node->next_.get(); } } }

pyohannes · 2020-07-21T19:21:10Z

+        }
+      }
+    }
+    return (int64_t)0;


Boost has a boost::blank type for this purpose. The more C++ idiomatic way would be top wrap the return value in a std::optional, but if feel skeptical about extending nostd by this (it only was introduced in C++17).

I also think this shouldn't block this PR.

pyohannes · 2020-07-21T19:22:25Z

+  }
+
+  // Returns the value associated with the passed in key.
+  context::ContextValue GetValue(nostd::string_view key)


This can be const and noexcept.

pyohannes · 2020-07-21T19:22:33Z

+  }
+
+  // Checks for key and returns true if found
+  bool HasKey(nostd::string_view key)


This can be const and noexcept.

pyohannes · 2020-07-21T19:37:46Z

+  // Returns the value associated with the passed in key.
+  context::ContextValue GetValue(nostd::string_view key)
+  {
+    for (DataList *data = &*head_; data != nullptr; data = &*(data->next_))


I think this is a bit more readable:

for (DataList *data = head_.get(); data != nullptr; data = data->next_.get())

Change made.

pyohannes · 2020-07-21T19:42:00Z

+  {
+    Context context = Context(values);
+    nostd::shared_ptr<DataList> last_node;
+    for (nostd::shared_ptr<DataList> data = context.head_; data != nullptr; data = data->next_)


This can be simplified a bit:

nostd::shared_ptr<DataList>& last = context.head_; while (last->next_ != nullptr) { last = last->next_; } last->next_ = head_;

pyohannes · 2020-07-21T19:42:33Z

+  // Checks for key and returns true if found
+  bool HasKey(nostd::string_view key)
+  {
+    for (nostd::shared_ptr<DataList> data = head_; data != nullptr; data = data->next_)


We can avoid copies of shared_ptr here:

for (DataList *data = head_.get(); data != nullptr; data = data->next_.get())

Change made.

pyohannes · 2020-07-21T19:44:31Z

I think we might need some memory testing (e.g. checking if all the allocated linked-list nodes are freed up before exiting the application (e.g. by using our custom malloc/free) to make sure we're actually doing the correct/efficient ref-counting. Probably in another PR.

I ran the unit test through valgrind, and it looks fine. It would be nice to have some automated memory checking though in our CI.

satac2 · 2020-07-21T20:12:02Z

Isn't there memory checking in the CI?

pyohannes

Thanks for all you efforts on this PR. I have one minor nit with the include lists, otherwise this is ready to merge.

pyohannes · 2020-07-21T21:12:06Z

+#include "opentelemetry/context/context_value.h"
+#include "opentelemetry/nostd/shared_ptr.h"
+#include "opentelemetry/nostd/string_view.h"
+#include "opentelemetry/trace/key_value_iterable_view.h"


Some include nits: I think atomic and key_value_iterable_view.h are not needed.

I have removed them.

pyohannes · 2020-07-21T21:46:07Z

Please rebase and then @reyang please merge.

Update dependency rules_go to v0.53.0

satac2 and others added 30 commits June 8, 2020 14:25

demo context header

7867296

demo context header

d5a360a

added file threadlocal_context.h

5d3b713

added fixes

d047ff7

Fixed syntatical errors, moved token class definition to context.h an…

f283cc6

…d added the contextvars_context.h file

Update api/include/opentelemetry/context/context.h

e1de13f

Co-authored-by: Kirk Kelsey <kirkmkelsey@gmail.com>

changed define guards and minor syntactical changes

d5b155d

removing context vars class

80b2332

Merge branch 'context_api' of github.com:satac2/opentelemetry-cpp int…

64cd1c2

…o context_api

removed threadlocal context and made the runtime context methods static

4d24115

removed threadlocal context and made the runtime context methods static

a17c028

added a constructor

d2df2ac

fixed convention issue and removed unnecessary code

c316636

Update context.h

7b36945

Update context.h

8e02c07

context implementation

115cd68

Adding tests

0d9fea1

Adding tests

565c40e

renamed some variables, added tests

2d4a411

renamed some variables, added tests

9874c4c

Merge branch 'context_api' into context_api_content

7bceaab

reversed public/private order

061e564

Merge branch 'context_api_content' of github.com:satac2/opentelemetry…

b5c81ad

…-cpp into context_api_content

Added detach test

b2717b3

avoiding standard datastructures

b7259cd

replaced std::map and created a thread_local context

34eacf2

replaced std::map and created a thread_local context

a086993

cleaned up

e44e926

cleaned up

6edce29

fixed undefined behavior in context.h

dff6f8f

pyohannes reviewed Jul 21, 2020

View reviewed changes

satac2 added 6 commits July 21, 2020 03:18

explicit conversion to contextValue in tests

86b032c

explicit type case in tests

f78ab74

added nostd::shared_ptr<SpanContext> to ContextValue

77c4c9a

removed span_context.h include, so tests would pass, not sure why

f63d595

readded span_context.h

5f25cf1

re-added span_context include

86eacca

reyang reviewed Jul 21, 2020

View reviewed changes

reyang approved these changes Jul 21, 2020

View reviewed changes

satac2 added 8 commits July 21, 2020 16:12

testing for the include failure

7a2ea4c

removed key_value_iterable_include

961f7ed

removed unncessary shared_ptr during iteration

4358d35

Merge branch 'master' into context_api_content

c5f6c08

added shared_ptr<trace::SpanContext> to context_value

80cbb96

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

896bc45

added shared_ptr<SpanContext> and include

feb3f17

Merge branch 'context_api_content' of github.com:satac2/opentelemetry…

acffb61

…-cpp into context_api_content

pyohannes reviewed Jul 21, 2020

View reviewed changes

simplifications

bf94af8

pyohannes approved these changes Jul 21, 2020

View reviewed changes

removed unecessary includes

5eddd2f

Merge branch 'master' into context_api_content

78bd8a6

reyang merged commit 37b275c into open-telemetry:master Jul 21, 2020

reyang mentioned this pull request Aug 25, 2020

Add context API and implementation #73

Closed

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

Merge pull request open-telemetry#180 from malkia/renovate/rules_go-0.x

0faf70b

Update dependency rules_go to v0.53.0

Conversation

satac2 commented Jul 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

reyang commented Jul 21, 2020

Uh oh!

pyohannes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pyohannes commented Jul 21, 2020

Uh oh!

satac2 commented Jul 21, 2020

Uh oh!

pyohannes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pyohannes commented Jul 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development