Skip to content
Merged
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
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@ endif()

include_directories(api/include)
add_subdirectory(api)
include_directories(sdk/include)
Comment thread
g-easy marked this conversation as resolved.
include_directories(sdk)
add_subdirectory(sdk)
add_subdirectory(examples)
4 changes: 0 additions & 4 deletions api/include/opentelemetry/plugin/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ class Span final : public trace::Span
{
span_->AddEvent(name, timestamp);
}
void AddEvent(nostd::string_view name, core::SteadyTimestamp timestamp) noexcept override
{
span_->AddEvent(name, timestamp);
}

void SetStatus(trace::CanonicalCode code, nostd::string_view description) noexcept override
{
Expand Down
1 change: 0 additions & 1 deletion api/include/opentelemetry/trace/noop.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ class NoopSpan final : public Span
void AddEvent(nostd::string_view name) noexcept override {}

void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override {}
void AddEvent(nostd::string_view name, core::SteadyTimestamp timestamp) noexcept override {}

void SetStatus(CanonicalCode code, nostd::string_view description) noexcept override {}

Expand Down
1 change: 0 additions & 1 deletion api/include/opentelemetry/trace/span.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ class Span

// Adds an event to the Span, with a custom timestamp.
virtual void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept = 0;
virtual void AddEvent(nostd::string_view name, core::SteadyTimestamp timestamp) noexcept = 0;

// TODO
// Adds an event to the Span, with a custom timestamp, and attributes.
Expand Down
2 changes: 0 additions & 2 deletions examples/plugin/plugin/tracer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ class Span final : public trace::Span

void AddEvent(nostd::string_view /*name*/, core::SystemTimestamp /*timestamp*/) noexcept override
{}
void AddEvent(nostd::string_view /*name*/, core::SteadyTimestamp /*timestamp*/) noexcept override
{}

void SetStatus(trace::CanonicalCode /*code*/,
nostd::string_view /*description*/) noexcept override
Expand Down
1 change: 1 addition & 0 deletions sdk/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
add_subdirectory(src)
1 change: 1 addition & 0 deletions sdk/src/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
add_subdirectory(trace)
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
25 changes: 25 additions & 0 deletions sdk/src/trace/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Copyright 2020, 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.

package(default_visibility = ["//visibility:public"])

cc_library(
name = "trace",
srcs = glob(["**/*.cc"]),
Comment thread
g-easy marked this conversation as resolved.
hdrs = glob(["**/*.h"]),
include_prefix = "src/trace",
deps = [
"//api",
],
)
1 change: 1 addition & 0 deletions sdk/src/trace/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
add_library(opentelemetry_trace tracer.cc span.cc)
48 changes: 48 additions & 0 deletions sdk/src/trace/recordable.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#pragma once

#include "opentelemetry/core/timestamp.h"
#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/trace/canonical_code.h"
#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
namespace trace_api = opentelemetry::trace;

/**
* Maintains a representation of a span in a format that can be processed by a recorder.
*
* This class is thread-compatible.
*/
class Recordable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think that we need to represent parenting on this level? Given this interface, I don't see how an exporter can determine a parent span or recordable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not all the methods are filled in (because we don't have all the types and methods in the api yet).

But there will be some kind of SpanContext type with a corresponding, SetSpanContext or something similar as well as AddLink methods. Those would pass the parenting information.

{
public:
virtual ~Recordable() = default;

/**
* Add an event to a span.
* @param name the name of the event
* @param timestamp the timestamp of the event
*/
virtual void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept = 0;

/**
* Set the status of the span.
* @param code the status code
* @param description a description of the status
*/
virtual void SetStatus(trace_api::CanonicalCode code,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we will have similar Recordable class for metrics and logs moving forward.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that would make sense. I find it useful to have interfaces that just capture events and don't mandate an accessor interface or maintaining data in a structured form.

nostd::string_view description) noexcept = 0;

/**
* Set the name of the span.
* @param name the name to set
*/
virtual void SetName(nostd::string_view name) noexcept = 0;
};
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
31 changes: 31 additions & 0 deletions sdk/src/trace/recorder.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#pragma once

#include "opentelemetry/trace/span_id.h"
#include "opentelemetry/trace/trace_id.h"
#include "opentelemetry/version.h"
#include "src/trace/recordable.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
class Recorder
{
public:
virtual ~Recorder() = default;

/**
* @return a Recordable object to maintain a data representation of a span.
*/
virtual std::unique_ptr<Recordable> MakeRecordable() noexcept = 0;

/**
* Record a span.
* @param recordable the data representation of the span.
*/
virtual void Record(std::unique_ptr<Recordable> &&recordable) noexcept = 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason for using std::unique_ptr instead of nonstd::unique_ptr in this interface?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was limiting nostd::unique_ptr to the places where we need a stable ABI that could cross DSOs.

I don't know that are plans to support dynamically loadable Recorders. I feel like we should try to limit the points where we support plug-ability to avoid confusion and we already have two that we've discussed targeting (i.e. Tracer plugins and Exporter plugins)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think Recorder should be an implementation detail of the SDK.

};
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
77 changes: 77 additions & 0 deletions sdk/src/trace/span.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#include "src/trace/span.h"

#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
Span::Span(std::shared_ptr<Tracer> &&tracer,
nostd::string_view name,
const trace_api::StartSpanOptions &options) noexcept
: tracer_{std::move(tracer)}, recordable_{tracer_->recorder().MakeRecordable()}
{
(void)options;
if (recordable_ == nullptr)
{
return;
}
recordable_->SetName(name);
}

Span::~Span()
{
End();
}

void Span::AddEvent(nostd::string_view name) noexcept
{
(void)name;
}

void Span::AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept
{
(void)name;
(void)timestamp;
}

void Span::SetStatus(trace_api::CanonicalCode code, nostd::string_view description) noexcept
{
std::lock_guard<std::mutex> lock_guard{mu_};
if (recordable_ == nullptr)
{
return;
}
recordable_->SetStatus(code, description);
}

void Span::UpdateName(nostd::string_view name) noexcept
{
std::lock_guard<std::mutex> lock_guard{mu_};
if (recordable_ == nullptr)
{
return;
}
recordable_->SetName(name);
}

void Span::End() noexcept
{
std::lock_guard<std::mutex> lock_guard{mu_};
if (recordable_ == nullptr)
{
return;
}
tracer_->recorder().Record(std::move(recordable_));
recordable_.reset();
}

bool Span::IsRecording() const noexcept
{
std::lock_guard<std::mutex> lock_guard{mu_};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the point of using a mutex here?
I guess this would only make sense when the caller has knowledge about the mutex, otherwise by the time the caller received the return value, it might not be the current status.

Copy link
Copy Markdown
Contributor Author

@rnburn rnburn Mar 12, 2020

Choose a reason for hiding this comment

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

I suppose the return value might not be accurate if you have other threads simultaneously ending the span. But it would be undefined behavior without it and this will at least prevent TSAN from flagging it as an error.

return recordable_ != nullptr;
}
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
46 changes: 46 additions & 0 deletions sdk/src/trace/span.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#pragma once

#include "opentelemetry/version.h"
#include "src/trace/tracer.h"

#include <mutex>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
namespace trace_api = opentelemetry::trace;

class Span final : public trace_api::Span
{
public:
explicit Span(std::shared_ptr<Tracer> &&tracer,
nostd::string_view name,
const trace_api::StartSpanOptions &options) noexcept;

~Span() override;

// trace_api::Span
void AddEvent(nostd::string_view name) noexcept override;

void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override;
Comment thread
g-easy marked this conversation as resolved.

void SetStatus(trace_api::CanonicalCode code, nostd::string_view description) noexcept override;

void UpdateName(nostd::string_view name) noexcept override;

void End() noexcept override;

bool IsRecording() const noexcept override;

trace_api::Tracer &tracer() const noexcept override { return *tracer_; }

private:
std::shared_ptr<Tracer> tracer_;
mutable std::mutex mu_;
std::unique_ptr<Recordable> recordable_;
Comment thread
g-easy marked this conversation as resolved.
};
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
20 changes: 20 additions & 0 deletions sdk/src/trace/tracer.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#include "src/trace/tracer.h"

#include "opentelemetry/version.h"
#include "src/trace/span.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
nostd::unique_ptr<trace_api::Span> Tracer::StartSpan(
nostd::string_view name,
const trace_api::StartSpanOptions &options) noexcept
{
return nostd::unique_ptr<trace_api::Span>{new (std::nothrow)
Span{this->shared_from_this(), name, options}};
}
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
32 changes: 32 additions & 0 deletions sdk/src/trace/tracer.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#pragma once

#include "opentelemetry/trace/tracer.h"
#include "opentelemetry/version.h"
#include "src/trace/recorder.h"

#include <memory>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
class Tracer final : public trace_api::Tracer, public std::enable_shared_from_this<Tracer>
{
public:
// Note: recorder must be non-null
explicit Tracer(std::unique_ptr<Recorder> &&recorder) noexcept : recorder_{std::move(recorder)} {}

Recorder &recorder() const noexcept { return *recorder_; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Implies the recorder passed to the constructor can't be nullptr, right? Is this worth documenting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added a note.


// trace_api::Tracer
nostd::unique_ptr<trace_api::Span> StartSpan(
nostd::string_view name,
const trace_api::StartSpanOptions &options = {}) noexcept override;

private:
std::unique_ptr<Recorder> recorder_;
};
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE