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

Resource sdk Implementation #502

Merged
merged 37 commits into from
Jan 22, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
a27a5e4
resource api
lalitb Jan 5, 2021
abe8287
format and add tests
lalitb Jan 5, 2021
083b169
fix bazel
lalitb Jan 5, 2021
b61acdd
fix bazel another try
lalitb Jan 5, 2021
4540bc5
more fixes
lalitb Jan 5, 2021
dfe857c
fix resouce interface constructor
lalitb Jan 5, 2021
0ae679e
Merge branch 'master' into resources
lalitb Jan 5, 2021
7673007
fix review comments
lalitb Jan 9, 2021
d5b389f
revert change
lalitb Jan 11, 2021
9fbe8be
revert change
lalitb Jan 11, 2021
208f231
revert change
lalitb Jan 11, 2021
60034d9
fix merge conflict
lalitb Jan 12, 2021
04e9de8
Merge branch 'master' into resources
lalitb Jan 12, 2021
bfdd7e1
fix attributes_utils.h
lalitb Jan 12, 2021
2abd049
fix review comments
lalitb Jan 12, 2021
f71a47c
fix attributes
lalitb Jan 12, 2021
f7e4909
add doc
lalitb Jan 12, 2021
bc3bebc
fix static
lalitb Jan 12, 2021
48a30d4
fix test
lalitb Jan 12, 2021
d8d015a
review comments
lalitb Jan 17, 2021
48ab9ea
fix bazel build
lalitb Jan 17, 2021
f515165
remove un-necesaary include
lalitb Jan 17, 2021
5b01258
add test for different resoure types
lalitb Jan 18, 2021
e9745f1
fix otlp
lalitb Jan 18, 2021
84d5405
Merge branch 'master' into resources
lalitb Jan 18, 2021
01fd510
fix valgrind error
lalitb Jan 18, 2021
c60fefc
wMerge branch 'resources' of github.com:lalitb/opentelemetry-cpp into…
lalitb Jan 18, 2021
b023c2a
modify simple trace example to use resource
lalitb Jan 18, 2021
1586018
fix simple example
lalitb Jan 18, 2021
9158d71
make resource ctor private
lalitb Jan 21, 2021
db2412b
Merge branch 'master' into resources
lalitb Jan 21, 2021
35dbf99
fix access
lalitb Jan 21, 2021
5e67443
Merge branch 'resources' of github.com:lalitb/opentelemetry-cpp into …
lalitb Jan 21, 2021
adbd86d
fixes
lalitb Jan 21, 2021
ef45994
fix comment
lalitb Jan 21, 2021
a272b92
Update sdk/src/resource/resource.cc
lalitb Jan 22, 2021
8cd247e
Merge branch 'master' into resources
lalitb Jan 22, 2021
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
70 changes: 70 additions & 0 deletions sdk/include/opentelemetry/sdk/resource/resource.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#pragma once

#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/nostd/unique_ptr.h"
#include "opentelemetry/sdk/trace/attribute_utils.h"
#include "opentelemetry/sdk/version/version.h"
#include "opentelemetry/version.h"

#include <cstdlib>
Copy link
Contributor

@ThomsonTan ThomsonTan Jan 13, 2021

Choose a reason for hiding this comment

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

Is this include necessary?

Copy link
Member Author

@lalitb lalitb Jan 13, 2021

Choose a reason for hiding this comment

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

No, we actually need it in resource.cpp ( to read env variable through std::getenv ). Will move it there. Thanks for noticing it.

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 is done now.

#include <memory>
#include <sstream>
#include <unordered_map>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace resource
{

class Resource
{
public:
Resource(const opentelemetry::common::KeyValueIterable &attributes) noexcept;

Resource(const opentelemetry::sdk::trace::AttributeMap &attributes) noexcept;

template <class T,
nostd::enable_if_t<common::detail::is_key_value_iterable<T>::value> * = nullptr>
Resource(const T &attributes) noexcept : Resource(common::KeyValueIterableView<T>(attributes))
{}

Resource(const std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>
attributes) noexcept
: Resource(nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>{
attributes.begin(), attributes.end()})
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's your opinion about replacing those with a single:

Resource(const std::map<std::string, opentelemetry::sdk::trace::SpanDataAttributeValue> attributes) noexcept;

As resources are an SDK only concept, we don't need to worry about ABI compatibility here.

Also, we should move the attribute_utils.h (containing the AttributeMap) from trace to common. Resources will be used by metrics as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point,
Using std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue directly, and getting rid of AttributeMap.


const opentelemetry::sdk::trace::AttributeMap &GetAttributes() const noexcept;

std::unique_ptr<Resource> Merge(const Resource &other);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIt: Here and elsewhere in this file. Can we add method documentation about the expectations of calling this?

E.g. for Merge I expect attributes already defined to not be overridden by other's attributes, but want to confirm this is expected.

For this review, yes I'm looking it up in the spec, but we should document it here for other maintainers of 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.

Sure. Will add the method documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added the documentation now.


static std::unique_ptr<Resource> Create(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have both Create methods and constructors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, probably because of creating unique pointers. Should we make the constructors private then, to prevent the creation of unmanaged resources?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is challenge to do this in C++. Ideally, I would like to make constructor as private, and make OTELResourceDetector as friend class of Resource, but we actually need all derivations of ResourceDetector have access to this constructor.
opentelementry-java does it using AutoValue annotation, and opentelemetry-dotnet does it using internal keyword.

Copy link
Member Author

@lalitb lalitb Jan 12, 2021

Choose a reason for hiding this comment

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

Ok. Please ignore above comment. I have now made constructor as protected ( so that it can be used in test cases ), and made OTELResourceDetector as friend class. It seems we don't need to invoke Resource constructor in any other detector, so we should be fine.

const opentelemetry::common::KeyValueIterable &attributes);

static std::unique_ptr<Resource> Create(
std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>
attributes) noexcept
{
return Create(nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>{
attributes.begin(), attributes.end()});
}

template <class T,
nostd::enable_if_t<common::detail::is_key_value_iterable<T>::value> * = nullptr>
static std::unique_ptr<Resource> Create(const T &attributes) noexcept
{
return Create(common::KeyValueIterableView<T>(attributes));
}

static std::unique_ptr<Resource> CreateEmpty();

static std::unique_ptr<Resource> Create(const std::string &attributes);

private:
const opentelemetry::sdk::trace::AttributeMap attribute_map_;
};

} // namespace resource
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
27 changes: 27 additions & 0 deletions sdk/include/opentelemetry/sdk/resource/resource_detector.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#pragma once

#include "opentelemetry/nostd/unique_ptr.h"
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace resource
{

class ResourceDetector
{
public:
virtual std::unique_ptr<opentelemetry::sdk::resource::Resource> Detect() = 0;
Copy link
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 to return a unique_ptr over just using the stack here?

Resource is non-virtual class, so it might be a simple API to just return it by-value.

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 the motivation behind that is to avoid excessive copying of a resource. A resource might be appended to each span. Having a shared_ptr (I think a unique pointer will not do) enables us to avoid copying the resource and its attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on suggestion from @jsuereth , Resources::Merge() and Resources::Create() are modified to return stack allocated Resource by value. And this resource is then owned by sdk::tracer_provider and passed as reference to span. Please refer to examples/simple/main.cc in this PR on how the usage would be, and let me know if any concerns.

};

class OTELResourceDetector : public ResourceDetector
{
public:
std::unique_ptr<opentelemetry::sdk::resource::Resource> Detect() noexcept override;
};

} // namespace resource
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
13 changes: 13 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/attribute_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,19 @@ class AttributeMap
return attributes_;
}

void AddAttributes(const AttributeMap &other)
{
for (auto &attr : other.attributes_)
{
if ((attributes_.find(attr.first) == attributes_.end()) ||
(nostd::holds_alternative<std::string>(attributes_[attr.first]) &&
nostd::get<std::string>(attributes_[attr.first]).size() == 0))
{
attributes_[attr.first] = attr.second;
}
}
}

void SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept
{
Expand Down
1 change: 1 addition & 0 deletions sdk/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ add_subdirectory(trace)
add_subdirectory(metrics)
add_subdirectory(logs)
add_subdirectory(version)
add_subdirectory(resource)
26 changes: 26 additions & 0 deletions sdk/src/resource/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# 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 = "resource",
srcs = glob(["**/*.cc"]),
hdrs = glob(["**/*.h"]),
include_prefix = "src/resource",
deps = [
"//api",
"//sdk:headers",
],
)
16 changes: 16 additions & 0 deletions sdk/src/resource/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
add_library(opentelemetry_resources resource.cc resource_detector.cc)

set_target_properties(opentelemetry_resources PROPERTIES EXPORT_NAME resources)

target_link_libraries(opentelemetry_resources opentelemetry_common)

target_include_directories(
opentelemetry_resources
PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/sdk/include>")

install(
TARGETS opentelemetry_resources
EXPORT "${PROJECT_NAME}-target"
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
74 changes: 74 additions & 0 deletions sdk/src/resource/resource.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/nostd/span.h"
#include "opentelemetry/sdk/resource/resource_detector.h"
#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace resource
{

const std::string TELEMETRY_SDK_LANGUAGE = "telemetry.sdk.language";
Copy link
Contributor

Choose a reason for hiding this comment

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

Name the constants like kTelemetrySdkLanguage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. fixed now :)

const std::string TELEMETRY_SDK_NAME = "telemetry.sdk.name";
const std::string TELEMETRY_SDK_VERSION = "telemetry.sdk.version";

Resource::Resource(const opentelemetry::common::KeyValueIterable &attributes) noexcept
: attribute_map_(attributes)
{}

Resource::Resource(const opentelemetry::sdk::trace::AttributeMap &attributes) noexcept
: attribute_map_(attributes)
{}

const opentelemetry::sdk::trace::AttributeMap &Resource::GetAttributes() const noexcept
{
return attribute_map_;
}

std::unique_ptr<Resource> Resource::Merge(const Resource &other)
{
opentelemetry::sdk::trace::AttributeMap merged_resource_attributes(other.GetAttributes());
merged_resource_attributes.AddAttributes(attribute_map_);
return std::unique_ptr<Resource>(new Resource(merged_resource_attributes));
}

std::unique_ptr<Resource> Resource::Create(
const opentelemetry::common::KeyValueIterable &attributes)
{
Resource default_resource({{{TELEMETRY_SDK_LANGUAGE, "cpp"},
{TELEMETRY_SDK_NAME, "opentelemetry"},
{TELEMETRY_SDK_VERSION, OPENTELEMETRY_SDK_VERSION}}});

if (attributes.size() > 0)
{
Resource input_resource(attributes);
auto merged_resource = default_resource.Merge(input_resource);
return merged_resource->Merge(*(OTELResourceDetector().Detect()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Would it make sense to have both the OTEL resource + default resource be statics within this function?

{
  static auto default_resource = Resource::GetDefault();
  static auto otel_env_resource = OTELResourceDetector().Detect();
  ...
}

This would avoid re-running the OTEL resource detection for every created resource.
  

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I have changed it accordingly.

  static auto otel_resource = OTELResourceDetector().Detect();
  auto default_resource     = Resource::GetDefault();

and

Resource &Resource::GetDefault()
{
  static Resource default_resource({{TELEMETRY_SDK_LANGUAGE, "cpp"},
                                    {TELEMETRY_SDK_NAME, "opentelemetry"},
                                    {TELEMETRY_SDK_VERSION, OPENTELEMETRY_SDK_VERSION}});
  return default_resource;
}

}
return default_resource.Merge(*(OTELResourceDetector().Detect()));
}

std::unique_ptr<Resource> Resource::CreateEmpty()
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 when we start to use resources, we have to make them shared pointers. Resources are supposed to be attached to every span. We don't want to have unique pointers there, but share a common resource with a shared pointer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Have changed std::unique_ptr to std::shared_ptr_ptr now.

Copy link
Member Author

@lalitb lalitb Jan 18, 2021

Choose a reason for hiding this comment

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

Ok. based on suggestion from @jsuereth , Resources::Merge() and Resources::Create() are modified to return stack allocated Resource by value. And this resource is then owned by sdk::tracer_provider and passed as reference to span. Please refer to examples/simple/main.cc in this PR on how the usage would be, and let me know if any concerns.

{
return Create({});
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Using static local for GetDefault() and GetEmpty() now.

}

std::unique_ptr<Resource> Resource::Create(const std::string &attributes)
{
opentelemetry::sdk::trace::AttributeMap resource_attributes;
std::istringstream iss(attributes);
std::string token;
while (std::getline(iss, token, ','))
{
size_t pos = token.find('=');
std::string key = token.substr(0, pos);
std::string value = token.substr(pos + 1);
resource_attributes.SetAttribute(key, value);
}
return std::unique_ptr<Resource>(new Resource(resource_attributes));
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 it would be better to have this logic in the OTELResourceDetector, as it's related to the format of the environment variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Have changed this accordingly now.

}

} // namespace resource
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
22 changes: 22 additions & 0 deletions sdk/src/resource/resource_detector.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#include "opentelemetry/sdk/resource/resource_detector.h"
#include <cstdlib>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace resource
{

const char *OTEL_RESOURCE_ATTRIBUTES = "OTEL_RESOURCE_ATTRIBUTES";

std::unique_ptr<Resource> OTELResourceDetector::Detect() noexcept
{
char *resource_attributes_str = std::getenv(OTEL_RESOURCE_ATTRIBUTES);
if (resource_attributes_str == nullptr)
return std::unique_ptr<Resource>(new Resource({}));
return Resource::Create(std::string(resource_attributes_str));
}

} // namespace resource
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
1 change: 1 addition & 0 deletions sdk/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ add_subdirectory(common)
add_subdirectory(trace)
add_subdirectory(metrics)
add_subdirectory(logs)
add_subdirectory(resource)
11 changes: 11 additions & 0 deletions sdk/test/resource/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
cc_test(
name = "resource_test",
srcs = [
"resource_test.cc",
],
deps = [
"//api",
"//sdk/src/resource",
"@com_google_googletest//:gtest_main",
],
)
9 changes: 9 additions & 0 deletions sdk/test/resource/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
foreach(testname resource_test)
add_executable(${testname} "${testname}.cc")
target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT} opentelemetry_resources)
gtest_add_tests(
TARGET ${testname}
TEST_PREFIX resources.
TEST_LIST ${testname})
endforeach()
Loading