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 19 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
76 changes: 76 additions & 0 deletions sdk/include/opentelemetry/sdk/resource/resource.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#pragma once

#include "opentelemetry/sdk/common/attribute_utils.h"
#include "opentelemetry/sdk/resource/resource_detector.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
{

using ResourceAttributes =
std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue>;

class Resource
{
public:
const ResourceAttributes &GetAttributes() const noexcept;

/**
* Returns a new, merged {@link Resource} by merging the current Resource
* with the other Resource. In case of a collision, current Resource takes
* precedence.
*
* @param other the Resource that will be merged with this.
* @returns the newly merged Resource.
*/

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

Choose a reason for hiding this comment

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

Do we have to implement these (internal to SDK surface) methods.. can we make it a map - inherited from standard library map (either map or unordered_map), and follow semantics of existing APIs available, e.g. map::merge .. Then we are not inventing some new Visual Basic-style API, but follow existing Standard Library semantics everywhere. This will be easier to comprehend by experienced modern C++ developer... Plus - I think it would be consistent if our container classes are compatible with existing algorithm templates described here

Copy link
Contributor

Choose a reason for hiding this comment

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

The spec defines some strict rules around merging, which I think are not satisfied by map::merge:

  • If the value on the primary resource is an empty string, the result has the value of the secondary resource.
  • Otherwise, the value of the primary resource is used.


/**
* Returns a newly created Resource with the specified attributes.
* It adds (merge) SDK attributes and OTEL attributes before returning.
* @param attributes for this resource
* @returns the newly created Resource.
*/

static std::shared_ptr<Resource> Create(const ResourceAttributes &attributes);

/**
* Returns an Empty resource.
*/

static Resource &GetEmpty();

/**
* Returns a Resource that indentifies the SDK in use.
*/

static Resource &GetDefault();

protected:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is protected necessary or just private is fine?

Copy link
Member Author

@lalitb lalitb Jan 21, 2021

Choose a reason for hiding this comment

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

protected helps creating unit test case - to subclass Resource and create instance of it for testing. We are doing this in resources_test.cc. I have modified the comment to indicate it's protected :)

/**
* The constructor is private and only for use internally by the class and
* inside ResourceDetector class.
* Users should use the Create factory method to obtain a Resource
* instance.
*/
Resource(const ResourceAttributes &attributes = ResourceAttributes()) noexcept;

private:
ResourceAttributes attributes_;

friend class OTELResourceDetector;
};

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

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

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace resource
{

class Resource;

/**
* Interface for a Resource Detector
*/
class ResourceDetector
{
public:
virtual std::shared_ptr<Resource> Detect() = 0;
};

/**
* OTelResourceDetector to detect the presence of and create a Resource
* from the OTEL_RESOURCE_ATTRIBUTES environment variable.
*/
class OTELResourceDetector : public ResourceDetector
{
public:
std::shared_ptr<Resource> Detect() noexcept override;
};

} // namespace resource
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
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})
69 changes: 69 additions & 0 deletions sdk/src/resource/resource.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#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 ResourceAttributes &attributes) noexcept : attributes_(attributes) {}

std::shared_ptr<Resource> Resource::Merge(const Resource &other) noexcept
{
ResourceAttributes merged_resource_attributes(attributes_);
for (auto &elem : other.attributes_)
{
if ((merged_resource_attributes.find(elem.first) == merged_resource_attributes.end()) ||
(nostd::holds_alternative<std::string>(attributes_[elem.first]) &&
nostd::get<std::string>(attributes_[elem.first]).size() == 0))
{
merged_resource_attributes[elem.first] = elem.second;
}
}
return std::make_shared<Resource>(Resource(merged_resource_attributes));
}

std::shared_ptr<Resource> Resource::Create(const ResourceAttributes &attributes)
{
static auto otel_resource = OTELResourceDetector().Detect();
auto default_resource = Resource::GetDefault();
// auto otel_resource = OTELResourceDetector().Detect();

if (attributes.size() > 0)
{
Resource tmp_resource(attributes);
auto merged_resource = tmp_resource.Merge(default_resource);
return merged_resource->Merge(*otel_resource);
}
return default_resource.Merge(*otel_resource);
}

Resource &Resource::GetEmpty()
{
static Resource empty_resource;
return empty_resource;
}

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

const ResourceAttributes &Resource::GetAttributes() const noexcept
{
return attributes_;
}

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

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace resource
{

const char *OTEL_RESOURCE_ATTRIBUTES = "OTEL_RESOURCE_ATTRIBUTES";

std::shared_ptr<Resource> OTELResourceDetector::Detect() noexcept
{
char *attributes_str = std::getenv(OTEL_RESOURCE_ATTRIBUTES);
if (attributes_str == nullptr)
return std::make_shared<Resource>(Resource());
// return Resource::GetEmpty();
ResourceAttributes attributes;
std::istringstream iss(attributes_str);
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);
attributes[key] = value;
}
return std::make_shared<Resource>(Resource(attributes));
}

} // 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