-
Notifications
You must be signed in to change notification settings - Fork 438
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
Changes from 8 commits
a27a5e4
abe8287
083b169
b61acdd
4540bc5
dfe857c
0ae679e
7673007
d5b389f
9fbe8be
208f231
60034d9
04e9de8
bfdd7e1
2abd049
f71a47c
f7e4909
bc3bebc
48a30d4
d8d015a
48ab9ea
f515165
5b01258
e9745f1
84d5405
01fd510
c60fefc
b023c2a
1586018
9158d71
db2412b
35dbf99
5e67443
adbd86d
ef45994
a272b92
8cd247e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
#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> | ||
#include <memory> | ||
#include <sstream> | ||
#include <unordered_map> | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
namespace sdk | ||
{ | ||
namespace resource | ||
{ | ||
|
||
using ResourceAttributes = | ||
std::unordered_map<std::string, opentelemetry::sdk::trace::SpanDataAttributeValue>; | ||
|
||
class Resource | ||
{ | ||
public: | ||
Resource(const ResourceAttributes &attributes = ResourceAttributes()) noexcept; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The specification says that ANY created resource should go through the environment variable additions + default logic you have in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Please ignore above comment. I have now made constructor |
||
|
||
const ResourceAttributes &GetAttributes() const noexcept; | ||
|
||
std::shared_ptr<Resource> Merge(const Resource &other); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just return a stack-allocated resource here? Wouldn't that be a simpler api? Similar for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it is possible to share resources across multiple spans, it would be better to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method doesn't need to determine how resources are shared. A few points:
I think it's reasonable for this method to return on the Stack. We then have our MetricProvider + TraceProvider have a Resources should really only be a startup concern and not something we expect users to pass around. I'd rather see a References:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, |
||
|
||
static std::shared_ptr<Resource> Create(const ResourceAttributes &attributes); | ||
|
||
static Resource &GetEmpty(); | ||
|
||
static Resource &GetDefault(); | ||
|
||
private: | ||
ResourceAttributes attributes_; | ||
}; | ||
|
||
} // namespace resource | ||
} // namespace sdk | ||
OPENTELEMETRY_END_NAMESPACE |
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::shared_ptr<opentelemetry::sdk::resource::Resource> Detect() = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same nit: Is there a reason to return a shared_ptr here? AFAIK Resource detection happens once and generally isn't passing pointers between different memory owners. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in earlier comment - As it is possible to share resources across multiple spans, it would be better to use shared_ptr and avoid un-necessary copying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to avoid the counting of shared_ptr references if we can just use the attached TraceProvider. I.e. shared_ptr has a cost, and I think we don't need it in this scenario. If we still want to use I think without seeing the other half of the change it's hard to fully comment on merits of shared_ptr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I did revisit again, and probably we mayn't need smart pointer here ( including unique_ptr). Let me add other half of implementation in this PR, so that it would be easy to comprehend and review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes are done as advised ( refer above comments ). |
||
}; | ||
|
||
class OTELResourceDetector : public ResourceDetector | ||
{ | ||
public: | ||
std::shared_ptr<opentelemetry::sdk::resource::Resource> Detect() noexcept override; | ||
}; | ||
|
||
} // namespace resource | ||
} // namespace sdk | ||
OPENTELEMETRY_END_NAMESPACE |
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", | ||
], | ||
) |
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}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
#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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name the constants like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
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) | ||
{ | ||
auto default_resource = Resource::GetDefault(); | ||
|
||
if (attributes.size() > 0) | ||
{ | ||
Resource tmp_resource(attributes); | ||
auto merged_resource = tmp_resource.Merge(default_resource); | ||
return merged_resource->Merge(*(OTELResourceDetector().Detect())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I have changed it accordingly.
and
|
||
} | ||
return default_resource.Merge(*(OTELResourceDetector().Detect())); | ||
} | ||
|
||
Resource &Resource::GetEmpty() | ||
{ | ||
static Resource empty; | ||
return empty; | ||
} | ||
|
||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
#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::shared_ptr<Resource> OTELResourceDetector::Detect() noexcept | ||
{ | ||
char *attributes_str = std::getenv(OTEL_RESOURCE_ATTRIBUTES); | ||
if (attributes_str == nullptr) | ||
return std::make_shared<Resource>(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 |
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", | ||
], | ||
) |
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() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
#include "opentelemetry/sdk/resource/resource.h" | ||
#include "opentelemetry/common/key_value_iterable_view.h" | ||
#include "opentelemetry/nostd/string_view.h" | ||
#include "opentelemetry/sdk/resource/resource_detector.h" | ||
#include "opentelemetry/sdk/trace/attribute_utils.h" | ||
|
||
#include <cstdlib> | ||
#include <string> | ||
#include <unordered_map> | ||
|
||
#include <gtest/gtest.h> | ||
|
||
TEST(ResourceTest, create) | ||
{ | ||
|
||
std::map<std::string, std::string> expected_attributes = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you supplement this with a test that handles strongly-typed attributes of numeric (integer) type? i.e. map of variants. Also - the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sure that would be good test to showcase. I will add it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have modified the test to include both integer and double resource attributes. Please see if this is fine. |
||
{"service", "backend"}, | ||
{"version", "1"}, | ||
{"cost", "234.23"}, | ||
{"telemetry.sdk.language", "cpp"}, | ||
{"telemetry.sdk.name", "opentelemetry"}, | ||
{"telemetry.sdk.version", OPENTELEMETRY_SDK_VERSION}}; | ||
auto resource = opentelemetry::sdk::resource::Resource::Create( | ||
{{"service", "backend"}, {"version", "1"}, {"cost", "234.23"}}); | ||
auto received_attributes = resource->GetAttributes(); | ||
|
||
for (auto &e : received_attributes) | ||
{ | ||
EXPECT_EQ(expected_attributes[e.first], opentelemetry::nostd::get<std::string>(e.second)); | ||
} | ||
EXPECT_EQ(received_attributes.size(), expected_attributes.size()); | ||
|
||
opentelemetry::sdk::resource::ResourceAttributes attributes = { | ||
{"service", "backend"}, {"version", "1"}, {"cost", "234.23"}}; | ||
auto resource2 = opentelemetry::sdk::resource::Resource::Create(attributes); | ||
auto received_attributes2 = resource2->GetAttributes(); | ||
for (auto &e : received_attributes2) | ||
{ | ||
EXPECT_TRUE(expected_attributes.find(e.first) != expected_attributes.end()); | ||
if (expected_attributes.find(e.first) != expected_attributes.end()) | ||
EXPECT_EQ(expected_attributes.find(e.first)->second, | ||
opentelemetry::nostd::get<std::string>(e.second)); | ||
} | ||
EXPECT_EQ(received_attributes2.size(), expected_attributes.size()); | ||
} | ||
|
||
TEST(ResourceTest, Merge) | ||
{ | ||
std::map<std::string, std::string> expected_attributes = {{"service", "backend"}, | ||
{"host", "service-host"}}; | ||
opentelemetry::sdk::resource::Resource resource1( | ||
opentelemetry::sdk::resource::ResourceAttributes({{"service", "backend"}})); | ||
opentelemetry::sdk::resource::Resource resource2( | ||
opentelemetry::sdk::resource::ResourceAttributes({{"host", "service-host"}})); | ||
|
||
auto merged_resource = resource1.Merge(resource2); | ||
auto received_attributes = merged_resource->GetAttributes(); | ||
for (auto &e : received_attributes) | ||
{ | ||
EXPECT_TRUE(expected_attributes.find(e.first) != expected_attributes.end()); | ||
if (expected_attributes.find(e.first) != expected_attributes.end()) | ||
EXPECT_EQ(expected_attributes.find(e.first)->second, | ||
opentelemetry::nostd::get<std::string>(e.second)); | ||
} | ||
EXPECT_EQ(received_attributes.size(), expected_attributes.size()); | ||
} | ||
|
||
TEST(ResourceTest, MergeEmptyString) | ||
{ | ||
std::map<std::string, std::string> expected_attributes = {{"service", "backend"}, | ||
{"host", "service-host"}}; | ||
opentelemetry::sdk::resource::Resource resource1({{"service", ""}, {"host", "service-host"}}); | ||
opentelemetry::sdk::resource::Resource resource2( | ||
{{"service", "backend"}, {"host", "another-service-host"}}); | ||
|
||
auto merged_resource = resource1.Merge(resource2); | ||
auto received_attributes = merged_resource->GetAttributes(); | ||
} | ||
|
||
// this test uses putenv to set the env variable - this is not available on windows | ||
#ifdef __linux__ | ||
TEST(ResourceTest, OtelResourceDetector) | ||
{ | ||
std::map<std::string, std::string> expected_attributes = {{"k", "v"}}; | ||
|
||
char env[] = "OTEL_RESOURCE_ATTRIBUTES=k=v"; | ||
putenv(env); | ||
|
||
opentelemetry::sdk::resource::OTELResourceDetector detector; | ||
auto resource = detector.Detect(); | ||
auto received_attributes = resource->GetAttributes(); | ||
for (auto &e : received_attributes) | ||
{ | ||
EXPECT_TRUE(expected_attributes.find(e.first) != expected_attributes.end()); | ||
if (expected_attributes.find(e.first) != expected_attributes.end()) | ||
EXPECT_EQ(expected_attributes.find(e.first)->second, | ||
opentelemetry::nostd::get<std::string>(e.second)); | ||
} | ||
EXPECT_EQ(received_attributes.size(), expected_attributes.size()); | ||
char env2[] = "OTEL_RESOURCE_ATTRIBUTES="; | ||
putenv(env2); | ||
} | ||
|
||
TEST(ResourceTest, OtelResourceDetectorEmptyEnv) | ||
{ | ||
std::map<std::string, std::string> expected_attributes = {}; | ||
char env[] = "OTEL_RESOURCE_ATTRIBUTES="; | ||
putenv(env); | ||
opentelemetry::sdk::resource::OTELResourceDetector detector; | ||
auto resource = detector.Detect(); | ||
auto received_attributes = resource->GetAttributes(); | ||
for (auto &e : received_attributes) | ||
{ | ||
EXPECT_TRUE(expected_attributes.find(e.first) != expected_attributes.end()); | ||
if (expected_attributes.find(e.first) != expected_attributes.end()) | ||
EXPECT_EQ(expected_attributes.find(e.first)->second, | ||
opentelemetry::nostd::get<std::string>(e.second)); | ||
} | ||
EXPECT_EQ(received_attributes.size(), expected_attributes.size()); | ||
} | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this include necessary?
There was a problem hiding this comment.
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 throughstd::getenv
). Will move it there. Thanks for noticing it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done now.