Logger: Propagating resources through LoggerProvider by owent · Pull Request #1154 · open-telemetry/opentelemetry-cpp

owent · 2021-12-20T16:30:45Z

Signed-off-by: owentou owentou@tencent.com

Changes

Add logs::LoggerContext, logs::MultiLogProcessor, logs::MultiRecordable
Remove resource from Logger::Log
Remove TracerProvider::GetProcessor (Without implementation before.)
[BREAK CHANGES] Change the declaration of sdk::logs::Recordable::SetResource
[BREAK CHANGES] Change the LoggerProvider::SetProcessor to LoggerProvider::AddProcessor
Share LoggerContext between loggers
Add api/include/opentelemetry/common/macros.h to provide OPENTELEMETRY_DEPRECATED, OPENTELEMETRY_DEPRECATED_MESSAGE and OPENTELEMETRY_MAYBE_UNUSED

Is there any way to convert opentelemetry::sdk::common::OwnedAttributeValue into opentelemetry::common::AttributeValue ?
It's difficult to reuse and dump data from opentelemetry::sdk::common::OwnedAttributeValue.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2021-12-20T16:38:39Z

Codecov Report

Merging #1154 (c53bfd1) into main (f20f72f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1154   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         174      174           
  Lines        6402     6402           
=======================================
  Hits         5972     5972           
  Misses        430      430

Impacted Files	Coverage Δ
...nclude/opentelemetry/sdk/common/empty_attributes.h	`100.00% <ø> (ø)`

lalitb · 2021-12-20T17:41:53Z

Is there any way to convert opentelemetry::sdk::common::OwnedAttributeValue into opentelemetry::common::AttributeValue ?
It's difficult to reuse and dump data from opentelemetry::sdk::common::OwnedAttributeValue.

Where do you see its usage? We can possibly create a converter similar to AttributeValue to OwnedAttributeValue -

opentelemetry-cpp/sdk/include/opentelemetry/sdk/common/attribute_utils.h

Line 65 in 7b9132c

struct AttributeConverter

owent · 2021-12-21T03:43:52Z

Is there any way to convert opentelemetry::sdk::common::OwnedAttributeValue into opentelemetry::common::AttributeValue ?
It's difficult to reuse and dump data from opentelemetry::sdk::common::OwnedAttributeValue.

Where do you see its usage? We can possibly create a converter similar to AttributeValue to OwnedAttributeValue -

opentelemetry-cpp/sdk/include/opentelemetry/sdk/common/attribute_utils.h

Line 65 in 7b9132c

struct AttributeConverter

I have wrote a example to share resource between logs before.And the codes is like these:

class KeyValueIterableViewForResourceAttributes final : public opentelemetry::common::KeyValueIterable {
 public:
  explicit KeyValueIterableViewForResourceAttributes(
      const opentelemetry::sdk::resource::ResourceAttributes& container) noexcept
      : container_{&container} {}

  // KeyValueIterable
  bool ForEachKeyValue(
      opentelemetry::nostd::function_ref<bool(nostd::string_view, opentelemetry::common::AttributeValue)> callback)
      const noexcept override {
    auto iter = std::begin(*container_);
    auto last = std::end(*container_);
    for (; iter != last; ++iter) {
      if (!callback(iter->first, ConvertOwnedAttribute(iter->second))) {
        return false;
      }
    }
    return true;
  }

  size_t size() const noexcept override { return nostd::size(*container_); }

  static opentelemetry::common::AttributeValue ConvertOwnedAttribute(
      const opentelemetry::sdk::common::OwnedAttributeValue& value) {
    if (nostd::holds_alternative<bool>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<bool>(value)};
    } else if (nostd::holds_alternative<int32_t>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<int32_t>(value)};
    } else if (nostd::holds_alternative<int64_t>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<int64_t>(value)};
    } else if (nostd::holds_alternative<uint32_t>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<uint32_t>(value)};
    } else if (nostd::holds_alternative<uint64_t>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<uint64_t>(value)};
    } else if (nostd::holds_alternative<double>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<double>(value)};
    } else if (nostd::holds_alternative<std::string>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<std::string>(value)};
    } else if (nostd::holds_alternative<std::vector<bool>>(value)) {
      // TODO: Add support for vector<bool>
      return opentelemetry::common::AttributeValue{};
    } else if (nostd::holds_alternative<std::vector<int32_t>>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<std::vector<int32_t>>(value)};
    } else if (nostd::holds_alternative<std::vector<uint32_t>>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<std::vector<uint32_t>>(value)};
    } else if (nostd::holds_alternative<std::vector<int64_t>>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<std::vector<int64_t>>(value)};
    } else if (nostd::holds_alternative<std::vector<uint64_t>>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<std::vector<uint64_t>>(value)};
    } else if (nostd::holds_alternative<std::vector<double>>(value)) {
      return opentelemetry::common::AttributeValue{nostd::get<std::vector<double>>(value)};
    } else if (nostd::holds_alternative<std::vector<std::string>>(value)) {
      // TODO: Add support to convert vector<std::string> tp vector<nostd::string_view>
      return opentelemetry::common::AttributeValue{};
    }

    return opentelemetry::common::AttributeValue{};
  }

 private:
  const opentelemetry::sdk::resource::ResourceAttributes* container_;
};

void Report(const opentelemetry::sdk::resource::Resource& shared_resource) {
  auto span = GetTracer()->StartSpan("cpp-test-span-otlp");
  auto ctx = span->GetContext();
  auto logger = GetLogger();
  auto shared_resource_attributes = KeyValueIterableViewForResourceAttributes(shared_resource.GetAttributes());
  using attribute_span = nostd::span<const std::pair<nostd::string_view, opentelemetry::common::AttributeValue>>;
  logger->Log(opentelemetry::logs::Severity::kInfo, "log name", "log body", shared_resource_attributes,
              opentelemetry::common::KeyValueIterableView<attribute_span>{attribute_span{}}, ctx.trace_id(),
              ctx.span_id(), ctx.trace_flags(), std::chrono::system_clock::now());
}

It can not convert all types from OwnedAttributeValue to AttributeValue . Even atfer move resource into provider, I think it's still useful if we want to Recordable::SetAttribute from data with lifetime(I want to reuse OwnedAttributeValue to store complex data structures and use it as attributes of trace or log).

lalitb · 2021-12-21T05:49:46Z

It can not convert all types from OwnedAttributeValue to AttributeValue . Even atfer move resource into provider, I think it's still useful if we want to Recordable::SetAttribute from data with lifetime(I want to reuse OwnedAttributeValue to store complex data structures and use it as attributes of trace or log).

Thanks for the explanation with a nicely written example. As you correctly mentioned, the scenario for resource is no more relevant once we start sharing resources through the provider. And passing resource as argument through Logger::log() should be removed. This is a breaking change, but acceptable as Logs Signal is still in preview. For log attributes, it would be better to pass them as KeyValueIterableView. Let me know if I am missing something here.

owent · 2021-12-21T06:08:54Z

It can not convert all types from OwnedAttributeValue to AttributeValue . Even atfer move resource into provider, I think it's still useful if we want to Recordable::SetAttribute from data with lifetime(I want to reuse OwnedAttributeValue to store complex data structures and use it as attributes of trace or log).

For log attributes, it would be better to pass them as KeyValueIterableView. Let me know if I am missing something here.

Yes, this is what I mean, and my question is how to create KeyValueIterableView of OwnedAttributeValue ?

lalitb · 2021-12-21T06:59:01Z

Yes, this is what I mean, and my question is how to create KeyValueIterableView of OwnedAttributeValue ?

I am still thinking why should this be under the scope of otel-cpp. Isn't this the responsibility of the library developer to ensure to call Span::AddEvent(name, attributes), Logger::Log(..., attributes,...) correctly? They may choose to use SDK::common::AttributMap as storage, or something as simple as std::map<std::string, std::string>. As long as it is converted into Key

Yes, this is what I mean, and my question is how to create KeyValueIterableView of OwnedAttributeValue ?

Which conversion is a problem in your above example - vector<std::string> to span<nostd::string_view> ? I think vector<bool> to span<bool> should be implicitly possible.

owent · 2021-12-21T07:23:20Z

Yes, this is what I mean, and my question is how to create KeyValueIterableView of OwnedAttributeValue ?

I am still thinking why should this be under the scope of otel-cpp. Isn't this the responsibility of the library developer to ensure to call Span::AddEvent(name, attributes), Logger::Log(..., attributes,...) correctly? They may choose to use SDK::common::AttributMap as storage, or something as simple as std::map<std::string, std::string>. As long as it is converted into Key

Yes, this is what I mean, and my question is how to create KeyValueIterableView of OwnedAttributeValue ?

Which conversion is a problem in your above example - vector<std::string> to span<nostd::string_view> ? I think vector<bool> to span<bool> should be implicitly possible.

vector<bool> has a special implementation, it is more likely a bitset and the elements in it have no unique address. span<bool> need contiguouse addresses for every element to initialize.

lalitb · 2021-12-21T07:34:46Z

vector<bool> has a special implementation, it is more likely a bitset and the elements in it have no unique address. span<bool> need contiguous addresses for every element to initialize.

yes agree, I see a problem with conversion for both vector<bool> and vector<string>, don't have a solution for now. Let me think about this. Now I also vaguely remember discussing this with @maxgolov during ETW implementation here.

owent · 2021-12-23T15:07:18Z

@lalitb This PR is almost finished. Could you please review the changes some time later?

lalitb

thanks for the PR. Changes are nicely done. have added initial comments. It's a large PR, will have another scan on the multi-processor, multi-recordable part.

lalitb · 2022-01-04T02:27:23Z

I don't have a strong opinion here, but Logger implementation is in preview and should be fine to have breaking changes without explicit deprecating first, and have cleaner code. Also for tracing implementation, specs are already stable, which means there won't be any further breaking changes required for it. Just thinking about whether we need to really add deprecated support.
Whatever we decide need to also be done for the instrumentation library cc @esigo

I think it's helpful for users to have some time to upgrade the usage of Log API instead of just compile error.

With logs in experimental, I think breaking changes should be acceptable and users should be prepared for it. This will keep the API much cleaner. We should definitely mention this as a breaking change in the Release description.

OK, I can remove these later, should I keep api/include/opentelemetry/common/macros.h ? Maybe it's useful in the future?

We can remove macros for now, can add them back later if required. Thanks.

lalitb · 2022-01-04T02:40:44Z

nit - we also need to update the benchmark submodule used by cmake, and version in the ./third_party_release file ( we can raise an issue, and do it in separate PR).

lalitb · 2022-01-04T03:03:36Z

Thanks for fixing this.

lalitb · 2022-01-05T01:02:48Z

What are we trying to achieve here - to ensure that the total time taken by all the processors shouldn't exceed timeout ? For traces, logic is a bit different, as the same timeout is used for all processors. This looks good too to me.

Yes, I think it may confuse ther users if they set timeout to some value, but actually take a more longer time.

E.g. when closing a service process, we may force to flush data, wait a limited time and then force to exit no matter it's done or not.

lalitb

LGTM once we remove the deprecated functionality.

owent · 2022-01-05T07:42:43Z

LGTM once we remove the deprecated functionality.

Done. I keep the OPENTELEMETRY_MAYBE_UNUSED and it's used in sdk/include/opentelemetry/sdk/common/empty_attributes.h . Some compiler will warning unused static function when it's included(maybe included by recordable.h) by source file and do not use it.

lalitb · 2022-01-06T18:18:23Z

LGTM once we remove the deprecated functionality.

Done. I keep the OPENTELEMETRY_MAYBE_UNUSED and it's used in sdk/include/opentelemetry/sdk/common/empty_attributes.h . Some compiler will warning unused static function when it's included(maybe included by recordable.h) by source file and do not use it.

Thanks. Probably all unused functionality can be removed separately some other time. Changes look good now. Thanks for working on this.

ThomsonTan · 2022-01-07T03:47:11Z

Why check vs2012 here? Seems the only valid case here is _MSVC_LANG >= 201703L. Also if this is true, should this be covered by the first check of __cplusplus >= 201703L?

Yes, it's duplicated, I have removed it.Thanks.

lalitb · 2022-01-12T06:51:39Z

@ThomsonTan - Is it good to merge?

Signed-off-by: owentou <owentou@tencent.com>

+ [BREAK CHANGES] Change the declaration of `sdk::logs::Recordable::SetResource` + [BREAK CHANGES] Change the `LoggerProvider::SetProcessor` into `LoggerProvider::AddProcessor` + Share `LoggerContext` between loggers Signed-off-by: owentou <owentou@tencent.com>

Signed-off-by: owentou <owentou@tencent.com>

+ Fix multi log processor + Fix crash problem when log with invalid severity Signed-off-by: owentou <owentou@tencent.com>

Signed-off-by: owentou <owentou@tencent.com>

owent · 2022-01-12T07:11:32Z

Rebase on main again

owent requested a review from a team December 20, 2021 16:30

owent force-pushed the propagating_resource_through_logger_provider branch from 9387847 to de2ebed Compare December 22, 2021 06:41

owent changed the title ~~[WIP] Logger: Propagating resources through LoggerProvider~~ Logger: Propagating resources through LoggerProvider Dec 23, 2021

owent force-pushed the propagating_resource_through_logger_provider branch from 19b3c7f to cfbb5f4 Compare January 4, 2022 02:17

lalitb reviewed Jan 4, 2022

View reviewed changes

owent force-pushed the propagating_resource_through_logger_provider branch 2 times, most recently from 32bbbf1 to ce61fda Compare January 4, 2022 06:20

lalitb reviewed Jan 5, 2022

View reviewed changes

lalitb approved these changes Jan 5, 2022

View reviewed changes

ThomsonTan reviewed Jan 7, 2022

View reviewed changes

owent added 8 commits January 12, 2022 15:10

[WIP] Add LoggerContext, MultiLogProcessor, MultiRecordable

0387582

Signed-off-by: owentou <owentou@tencent.com>

[WIP]

88639eb

+ [BREAK CHANGES] Change the declaration of `sdk::logs::Recordable::SetResource` + [BREAK CHANGES] Change the `LoggerProvider::SetProcessor` into `LoggerProvider::AddProcessor` + Share `LoggerContext` between loggers Signed-off-by: owentou <owentou@tencent.com>

Rebase from v1.1.1

c54571f

Signed-off-by: owentou <owentou@tencent.com>

Remove resource from Logger::Log(...)

89ca889

Signed-off-by: owentou <owentou@tencent.com>

Fix ElasticSearchRecordable

30bded1

Signed-off-by: owentou <owentou@tencent.com>

+ Fix compiling problem with gcc 11(benchmark)

d01c1cf

+ Fix multi log processor + Fix crash problem when log with invalid severity Signed-off-by: owentou <owentou@tencent.com>

Fix address sanitizer of OtlpHttpClient and MultiLogProcessor

424b649

Signed-off-by: owentou <owentou@tencent.com>

Rename macros

fab75be

Signed-off-by: owentou <owentou@tencent.com>

owent added 3 commits January 12, 2022 15:10

Fix spell error

d6c09ce

Signed-off-by: owentou <owentou@tencent.com>

Remove OPENTELEMETRY_DEPRECATED and OPENTELEMETRY_DEPRECATED_MESSAGE

8d80cbb

Signed-off-by: owentou <owentou@tencent.com>

Remove duplicated check for VS 2012

c53bfd1

Signed-off-by: owentou <owentou@tencent.com>

owent force-pushed the propagating_resource_through_logger_provider branch from 6d72128 to c53bfd1 Compare January 12, 2022 07:11

ThomsonTan approved these changes Jan 12, 2022

View reviewed changes

ThomsonTan merged commit d206b50 into open-telemetry:main Jan 12, 2022

Uh oh!

Conversation

owent commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov Bot commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lalitb commented Dec 20, 2021

Uh oh!

owent commented Dec 21, 2021

Uh oh!

lalitb commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owent commented Dec 21, 2021

Uh oh!

lalitb commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owent commented Dec 21, 2021

Uh oh!

lalitb commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owent commented Dec 23, 2021

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

lalitb Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

owent commented Jan 5, 2022

Uh oh!

lalitb commented Jan 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb commented Jan 12, 2022

Uh oh!

owent commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

owent commented Dec 20, 2021 •

edited

Loading

codecov Bot commented Dec 20, 2021 •

edited

Loading

lalitb commented Dec 21, 2021 •

edited

Loading

lalitb commented Dec 21, 2021 •

edited

Loading

lalitb commented Dec 21, 2021 •

edited

Loading

lalitb Jan 4, 2022 •

edited

Loading