Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Draft implementation of Context. #200

Merged
merged 4 commits into from
Sep 27, 2018
Merged

Conversation

g-easy
Copy link
Contributor

@g-easy g-easy commented Sep 26, 2018

Context holds information specific to an operation, such as a TagMap and
Span. Each thread has a currently active Context. Contexts are conceptually
immutable: the contents of a Context cannot be modified in-place.

Context holds information specific to an operation, such as a TagMap and
Span. Each thread has a currently active Context. Contexts are conceptually
immutable: the contents of a Context cannot be modified in-place.
opencensus::context::WithContext wc(std::move(ctx));
}

#if 0

Choose a reason for hiding this comment

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

Comment why 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

class Context {
public:
// Creates a default Context.
Context();

Choose a reason for hiding this comment

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

Need this public? Please make it private until we need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// WithContext is an RAII object, only ever stack-allocate it. While it's in
// scope, the execution will happen under a copy of the given Context.
class WithContext {

Choose a reason for hiding this comment

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

We probably need a version of this that accepts a boolean condition to avoid duplicate code:

if (condition) {
WithContext wc(ctx);
... // code
} else {
... // same code
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #202

WithContext& operator=(const WithContext&) = delete;
WithContext& operator=(WithContext&&) = delete;

Context swapped_context_;

Choose a reason for hiding this comment

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

Based on some experience that I have from dealing with other context implementation it is better if we have a way to check that objects are allocated and deallocated on the same thread. I suggest to put this check under the NDEBUG macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #203

@g-easy g-easy merged commit bd2b668 into census-instrumentation:master Sep 27, 2018
@g-easy g-easy deleted the ctx branch September 27, 2018 18:57
@g-easy g-easy mentioned this pull request Oct 3, 2018
10 tasks

// Shared pointer to the underlying Span representation. This is nullptr for
// Spans which are not recording events. This is an implementation detail, not
// part of the public API.
const std::shared_ptr<SpanImpl> span_impl_;
std::shared_ptr<SpanImpl> span_impl_;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We made the shared_ptr non-const (and also the SpanContext above) so that we can use swap() (L170) and operator= (#191) to modify a Span object in-place, so that Context can have a Span member (as opposed to pointer to Span).

Also in (#191) we marked all Span methods const because:

  1. They don't act on the shared_ptr, they act on the pointee.
  2. In future we can replace the shared_ptr with something like opencensus-java's approach.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants