Update logs sdk and api to follow specification #1884

owent · 2022-12-22T11:58:20Z

Fixes #1692
Fixes #1693
Fixes #1687
Fixes #1688

Changes

Add include_trace_context and scope attribute for logs API and SDK.
Add type trait to set field of LogRecord.
Add EventLoggerProvider and LoggerProvider

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 · 2022-12-22T12:13:40Z

Codecov Report

Merging #1884 (d336df3) into main (7e7184d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1884   +/-   ##
=======================================
  Coverage   87.12%   87.12%           
=======================================
  Files         165      165           
  Lines        4596     4596           
=======================================
  Hits         4004     4004           
  Misses        592      592

Impacted Files	Coverage Δ
.../include/opentelemetry/common/key_value_iterable.h	`100.00% <ø> (ø)`

Signed-off-by: WenTao Ou <admin@owent.net>

Signed-off-by: owent <admin@owent.net>

lalitb · 2023-01-04T21:05:00Z

Sorry for delay on this. Will provide my comments in this week.

owent · 2023-01-05T03:22:27Z

Sorry for delay on this. Will provide my comments in this week.

Thanks, should we mark the old Logger::Log() APIs as deprecated?The new Logger::EmitLogRecord() and EventLogger::EmitEvent() accept the SpanID, TraceID, Attributes and etc in any order.

lalitb · 2023-01-10T07:00:44Z

@owent - Do you think this issue - open-telemetry/opentelemetry-specification#3086 - will bring any changes in the Event API/SDK implementation ?

lalitb · 2023-01-10T07:13:23Z

should we mark the old Logger::Log() APIs as deprecated?The new Logger::EmitLogRecord() and EventLogger::EmitEvent() accept the SpanID, TraceID, Attributes and etc in any order.

I thought Logger::Log() API provides single line convenience API to instrumentation libraries for emitting logs, and we should keep them as it is.

owent · 2023-01-10T11:13:30Z

@owent - Do you think this issue - open-telemetry/opentelemetry-specification#3086 - will bring any changes in the Event API/SDK implementation ?

Not yet. This PR implement the main branch of https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md by now.And the EventLogger is a standalone component.
Maybe some APIs of EventLogger may change in the future, I can continue modify them then.

owent · 2023-01-10T11:15:44Z

should we mark the old Logger::Log() APIs as deprecated?The new Logger::EmitLogRecord() and EventLogger::EmitEvent() accept the SpanID, TraceID, Attributes and etc in any order.

I thought Logger::Log() API provides single line convenience API to instrumentation libraries for emitting logs, and we should keep them as it is.

OK, we can keep Logger::Log(), just wondering should we also modify the parameters so SpanID, TraceID, Attributes and etc and be passed in any order, just like Logger::EmitLogRecord()?

api/include/opentelemetry/logs/event_logger.h

Signed-off-by: owent <admin@owent.net>

lalitb · 2023-01-18T00:39:57Z

api/include/opentelemetry/logs/logger.h

+   * @return nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>
+   */
+  inline static nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>
+  MakeAttributes(std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>


Won't this will have usage inconsistency across Trace and Logs. In Trace API, we can pass initializer_list directly as argument (due to use of overloaded methods), and in logs we have to use MakeAttributes() to convert and pass ?

initializer_list can not be detected by universial reference(ArgumentType &&).Is there any better suggestion here?

What if we remove the variadic template argument from EmitLogRecord() and EmitEvent(), and have a method in LogRecord to add attributes?

LogRecord::SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) LogRecord::SetAttributes(const common::KeyValueIterable &attributes)

LogRecord already has SetAttribute. I think it's easier to use for users if they do not need to emit log in just one call. Just like Logger::Log, users don't need CreateLogRecord() and then SetAttribute().
MakeAttributes can be used to set attributes easier.

logger->EmitLogRecord(Severity::kFatal, "Logging an initializer list", Logger::MakeAttributes({{"key1", "value 1"}, {"key2", 2}}));

Thanks for explanation. For logger API, we will now have two different methods doing similar thing:

logger->Log(Severity::kFatal, "Logging an initializer list", {{"key1", "value 1"}, {"key2", 2}})

And

logger->EmitLogRecord(Severity::kFatal, "Logging an initializer list", Logger::MakeAttributes({{"key1", "value 1"}, {"key2", 2}}));

And the existing (first) one is much simpler. Or did you planned to remove the Logger::Log() method after adding the new method ?

I have the same question above( #1884 (comment)_ ). I think we can discuss if we should keep Logger::Log.

I also don't see advantage having both the methods. We can get rid of Logger::Log. Thanks for clarifying.

Thanks, Logger::Log is removed now.

api/include/opentelemetry/logs/event_logger.h

api/test/logs/provider_test.cc

…everity>`. Signed-off-by: WenTao Ou <admin@owent.net>

Signed-off-by: WenTao Ou <admin@owent.net>

api/include/opentelemetry/logs/logger.h

Signed-off-by: WenTao Ou <admin@owent.net>

lalitb

LGTM Thanks.

api/include/opentelemetry/logs/logger.h

Signed-off-by: WenTao Ou <admin@owent.net>

lalitb · 2023-01-26T07:00:06Z

Thanks @owent for fixing all the comments. This PR was discussed in today's community meeting. As we are using variadic templates/pack-paraments, and other meta-programming concepts in the code, there was interest for other community members to have a look into the PR. The suggestion was to keep this PR open for review till next community meeting (i.e, 30th Jan) for sufficient time for others to review and comment if required :)

owent · 2023-01-26T08:38:51Z

Thanks @owent for fixing all the comments. This PR was discussed in today's community meeting. As we are using variadic templates/pack-paraments, and other meta-programming concepts in the code, there was interest for other community members to have a look into the PR. The suggestion was to keep this PR open for review till next community meeting (i.e, 30th Jan) for sufficient time for others to review and comment if required :)

Thanks.

marcalff

Ok with the coding patterns used.

marcalff · 2023-01-26T15:30:07Z

api/include/opentelemetry/logs/event_logger.h

+  template <class... ArgumentType>
+  void EmitEvent(nostd::string_view event_name, ArgumentType &&... args)
+  {
+    nostd::shared_ptr<Logger> delegate_logger = GetDelegateLogger();
+    if (!delegate_logger)
+    {
+      return;
+    }
+    nostd::unique_ptr<LogRecord> log_record = delegate_logger->CreateLogRecord();
+    if (!log_record)
+    {
+      return;
+    }
+
+    IgnoreTraitResult(
+        detail::LogRecordSetterTrait<typename std::decay<ArgumentType>::type>::template Set(
+            log_record.get(), std::forward<ArgumentType>(args))...);
+
+    EmitEvent(event_name, std::move(log_record));
+  }


@owent

So, with the use of variadic templates, type traits, lamda expressions in the implementation, etc ... there is a considerable complexity here.

Because this is in the public API, users trying to figure out how to use this might have some questions and will need a clear documentation in opentelemetry.io with examples, and I include myself in "users" here.

This also increases complexity for maintenance, because if this breaks, understanding all the details and providing a fix will take more time, compared to more trivial C++ code.

I assume that the complexity here also saves from some repetitive code that would be also hard to maintain otherwise, so there are plus sides as well.

Overall, the complexity argument does not make the code invalid, so I view it as:

subjective (people with different experiences with these patterns will have different views)

and not objective (a given area in the code is technically broken and needs a fix)

To confirm, ok with me to have this code merged to main, knowing fully that I would need to improve my own variadic template skills as a pre requisite if I ever have to make changes here.

LGTM for this part, did not check everything else yet.

Thanks for your feedback. Here is my thinking about this complexity.There is a little different between trace and log APIs, with trace, we always need a variable to hold the lifetime of span(Span or Scope) and decide when to call End() by RAII, so we can set data field of Span one by by one after we create the span.
But in the use case of logs, it's easier for users(I'm also a user of both log and trace.) to use by call just one function, this function is Logger::Log() before and Logger::EmitLogRecord() now.There are also many data fields we need set to LogRecord, log body, trace information, attributes, timestamp and so on.There are three ways to set attributes before(By const T&, std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>, const common::KeyValueIterable &), and user also can decide whether to set other data fields, log body or trace information. Just consider log body, trace context and attributes, there will be more than 2*2*3+2*2=16 possibilities.Actually, we had more than 10 overloads of Logger::Log() before.If we have more options for logs, then the overloads will increase exponentially(I'm trying to add TraceContext now, according to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/api.md#logrecord).It will also be hard to maintain these overloads, especially we may add more options in the future.

I think we can disscuss more deeply if there is any better idea about how to solve this problem.

The approach of this PR try to use template <class ValueType> struct LogRecordSetterTrait; to implement these overloads by type, and also ignore the argument order.We can use std::decay<ArgumentType>::type to remove const, reference and violate of ArgumentType and use it as the template argument of template <class ValueType>, so we can implement any overload action by just write one template specialization for LogRecordSetterTrait. And the template <class ArgumentType> inline static LogRecord *Set(LogRecord *log_record, ArgumentType &&arg) noexcept use the universal reference so it can accept both left value reference and right value with or without const or violate.

Thanks @owent for the details.

This seems the best solution then ... and great work.

esigo

LGTM
Thanks for the great work :)
I agree that, maintaining this will be challenging but I see no better option.

sdk/test/logs/logger_provider_sdk_test.cc

owent requested a review from a team December 22, 2022 11:58

owent changed the title ~~[WIP] Update logs sdk and api to follow specification~~ Update logs sdk and api to follow specification Dec 22, 2022

owent force-pushed the update_logs_sdk branch from 55ed2ef to 41da3ac Compare December 24, 2022 05:32

owent added 4 commits December 24, 2022 13:34

Add include_trace_context and attributes for Logs APIs

1d1ab69

Signed-off-by: WenTao Ou <admin@owent.net>

Add test of creating logger with scope attributes.

d89be51

Signed-off-by: WenTao Ou <admin@owent.net>

Add EventLogger.

bdbcd4d

Signed-off-by: WenTao Ou <admin@owent.net>

Fix -Wextra warnings.

7c1ec27

Signed-off-by: owent <admin@owent.net>

owent force-pushed the update_logs_sdk branch from 41da3ac to 7c1ec27 Compare December 24, 2022 05:35

esigo assigned lalitb and esigo Jan 2, 2023

Merge branch 'main' into update_logs_sdk

8a5e09d

Merge branch 'main' into update_logs_sdk

1716b08

Merge branch 'main' into update_logs_sdk

827b05c

lalitb reviewed Jan 13, 2023

View reviewed changes

api/include/opentelemetry/logs/event_logger.h Outdated Show resolved Hide resolved

api/include/opentelemetry/logs/event_logger.h Outdated Show resolved Hide resolved

api/include/opentelemetry/logs/event_logger.h Show resolved Hide resolved

owent added 2 commits January 13, 2023 15:08

Merge remote-tracking branch 'opentelemetry/main' into update_logs_sdk

2a78338

Remove unused headers and EmitEvent API.

92806cd

Signed-off-by: owent <admin@owent.net>

lalitb reviewed Jan 18, 2023

View reviewed changes

owent added 3 commits January 20, 2023 14:58

Merge remote-tracking branch 'opentelemetry/main' into update_logs_sdk

d7bd806

Remove EventLogger::CreateLogRecord and add `LogRecordSetterTrait<S…

4b9741a

…everity>`. Signed-off-by: WenTao Ou <admin@owent.net>

Remove string options from GetLogger.

161631b

Signed-off-by: WenTao Ou <admin@owent.net>

lalitb reviewed Jan 21, 2023

View reviewed changes

api/include/opentelemetry/logs/logger.h Outdated Show resolved Hide resolved

lalitb reviewed Jan 21, 2023

View reviewed changes

api/include/opentelemetry/logs/logger.h Outdated Show resolved Hide resolved

Remove Logger::Log, add comment document for log ArgumentType.

dc63863

Signed-off-by: WenTao Ou <admin@owent.net>

lalitb approved these changes Jan 24, 2023

View reviewed changes

api/include/opentelemetry/logs/logger.h Outdated Show resolved Hide resolved

owent added 2 commits January 24, 2023 21:56

Merge remote-tracking branch 'opentelemetry/main' into update_logs_sdk

22334ba

Do not allow pass Severity to Debug/Trace/Info/Error....

bfdbf62

Signed-off-by: WenTao Ou <admin@owent.net>

marcalff reviewed Jan 26, 2023

View reviewed changes

Merge branch 'main' into update_logs_sdk

d336df3

esigo approved these changes Jan 28, 2023

View reviewed changes

sdk/test/logs/logger_provider_sdk_test.cc Show resolved Hide resolved

esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Jan 28, 2023

marcalff approved these changes Jan 28, 2023

View reviewed changes

lalitb merged commit 65ebdb1 into open-telemetry:main Jan 28, 2023

owent mentioned this pull request Feb 16, 2023

Prepare release, include fixes with abi breaking changes #1983

Closed

owent mentioned this pull request May 25, 2023

Remove LogName argument as deprecated from Logger API #1421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update logs sdk and api to follow specification #1884

Update logs sdk and api to follow specification #1884

owent commented Dec 22, 2022 •

edited

Loading

codecov bot commented Dec 22, 2022 •

edited

Loading

lalitb commented Jan 4, 2023

owent commented Jan 5, 2023

lalitb commented Jan 10, 2023

lalitb commented Jan 10, 2023

owent commented Jan 10, 2023

owent commented Jan 10, 2023

lalitb Jan 18, 2023

owent Jan 19, 2023

lalitb Jan 19, 2023 •

edited

Loading

owent Jan 20, 2023

lalitb Jan 20, 2023 •

edited

Loading

owent Jan 20, 2023

lalitb Jan 21, 2023 •

edited

Loading

owent Jan 22, 2023

lalitb left a comment

lalitb commented Jan 26, 2023

owent commented Jan 26, 2023

marcalff left a comment

marcalff Jan 26, 2023

owent Jan 28, 2023 •

edited

Loading

marcalff Jan 28, 2023

esigo left a comment

Update logs sdk and api to follow specification #1884

Update logs sdk and api to follow specification #1884

Conversation

owent commented Dec 22, 2022 • edited Loading

Changes

codecov bot commented Dec 22, 2022 • edited Loading

Codecov Report

lalitb commented Jan 4, 2023

owent commented Jan 5, 2023

lalitb commented Jan 10, 2023

lalitb commented Jan 10, 2023

owent commented Jan 10, 2023

owent commented Jan 10, 2023

lalitb Jan 18, 2023

Choose a reason for hiding this comment

owent Jan 19, 2023

Choose a reason for hiding this comment

lalitb Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

owent Jan 20, 2023

Choose a reason for hiding this comment

lalitb Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

owent Jan 20, 2023

Choose a reason for hiding this comment

lalitb Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

owent Jan 22, 2023

Choose a reason for hiding this comment

lalitb left a comment

Choose a reason for hiding this comment

lalitb commented Jan 26, 2023

owent commented Jan 26, 2023

marcalff left a comment

Choose a reason for hiding this comment

marcalff Jan 26, 2023

Choose a reason for hiding this comment

owent Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

marcalff Jan 28, 2023

Choose a reason for hiding this comment

esigo left a comment

Choose a reason for hiding this comment

owent commented Dec 22, 2022 •

edited

Loading

codecov bot commented Dec 22, 2022 •

edited

Loading

lalitb Jan 19, 2023 •

edited

Loading

lalitb Jan 20, 2023 •

edited

Loading

lalitb Jan 21, 2023 •

edited

Loading

owent Jan 28, 2023 •

edited

Loading