New LogRecord and Recordable implementations.

[owent]

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

Fixes #1691
Fixes #1689

Changes

There are a lot break changes in this PR.But it's easy to migrate if users do not implement their own Logger and Exporter.

• Removed deprecated Logger::Log APIs.
  • All APIs with the old name field are removed now.The name field is ognored from v1.4.0.
• New LogRecord, ReadableLogRecord, ReadWriteLogRecord
  • https://github.com/open-telemetry/opentelemetry-specification/blob/v1.15.0/specification/logs/api.md#logrecord
  • https://github.com/open-telemetry/opentelemetry-specification/blob/v1.15.0/specification/logs/sdk.md#additional-logrecord-interfaces
• New logs::Recordable to collect data from the all in one LogRecord.
  • [Logs SDK] Rename and update existing LogRecord class, modify existing Recordable interface #1691
• Add a simple EmitEvent function which will be use to implement [Logs API] LoggerProvider changes #1687 and [Logs API] - Logger Changes #1688 in the future.
  • https://github.com/open-telemetry/opentelemetry-specification/blob/v1.15.0/specification/logs/api.md#emit-event
  • It's not finished yet, and will be continued in another PR later.
• Replace std::unique_ptr<T> with nostd::unique_ptr<T> in logs.

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]

Codecov Report

Merging #1766 (03ce954) into main (f3daca0) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1766      +/-   ##
==========================================
- Coverage   85.73%   85.71%   -0.01%     
==========================================
  Files         171      171              
  Lines        5240     5240              
==========================================
- Hits         4492     4491       -1     
- Misses        748      749       +1     

Impacted Files	Coverage Δ
...ude/opentelemetry/common/key_value_iterable_view.h	88.89% <ø> (ø)
api/include/opentelemetry/nostd/shared_ptr.h	100.00% <ø> (ø)
...ude/opentelemetry/exporters/ostream/common_utils.h	100.00% <ø> (ø)
...include/opentelemetry/sdk/common/circular_buffer.h	100.00% <ø> (ø)
sdk/src/trace/batch_span_processor.cc	90.63% <0.00%> (-0.78%)	⬇️

[lalitb]

All APIs with the old name field are removed now.

Do you think we can keep the existing API intact (maybe renaming to Logger::EmitLog(..., ..., ...)) as mentioned in #1688. As per the logging API specs, the newly introduced interface to emit the log-records is only for Appenders. The specs doesn't enforce it for instrumentation library which makes sense because most of the languages (except C++) have logging api as part of language specs. The existing API can continue using direct Recordable interface to send the logs to exporter, without creating LogRecord object. Sometime, instrumentation library author would prefer to write a single line of code to emit logs.

And then the new interface can build the Recordable through ReadWriteLogRecord as you are doing right now in this PR.

[owent]

intact

All APIs with the old name field are removed now.

Do you think we can keep the existing API intact (maybe renaming to Logger::EmitLog(..., ..., ...)) as mentioned in #1688. As per the logging API specs, the newly introduced interface to emit the log-records is only for Appenders. The specs doesn't enforce it for instrumentation library which makes sense because most of the languages (except C++) have logging api as part of language specs. The existing API can continue using direct Recordable interface to send the logs to exporter, without creating LogRecord object. Sometime, instrumentation library author would prefer to write a single line of code to emit logs.

And then the new interface can build the Recordable through ReadWriteLogRecord as you are doing right now in this PR.

Sorry, I'm poor in English and perhaps I didn't make it clear. The name field means All Logger::Log with name parameter, this argument is ignored, for example OPENTELEMETRY_DEPRECATED_MESSAGE("name will be removed in the future") void Log(Severity severity, nostd::string_view name, nostd::string_view message) noexcept. We have discussed at #1383 before, and I think it's long enough to leave time for users to migrate now.

For users, we can still use the old Logger::Log APIs to emit logs, if we allow processor or exporters to use Recordable interface to send the logs without LogRecord, we can make Recordable derive from LogRecord. But if we changes or add more fields into LogRecord , we have to modify all Recordable implementations. Do you think it will be more difficult to maintain and upgrade for users?

[lalitb]

intact

All APIs with the old name field are removed now.

Do you think we can keep the existing API intact (maybe renaming to Logger::EmitLog(..., ..., ...)) as mentioned in #1688. As per the logging API specs, the newly introduced interface to emit the log-records is only for Appenders. The specs doesn't enforce it for instrumentation library which makes sense because most of the languages (except C++) have logging api as part of language specs. The existing API can continue using direct Recordable interface to send the logs to exporter, without creating LogRecord object. Sometime, instrumentation library author would prefer to write a single line of code to emit logs.
And then the new interface can build the Recordable through ReadWriteLogRecord as you are doing right now in this PR.

Sorry, I'm poor in English and perhaps I didn't make it clear. The name field means All Logger::Log with name parameter, this argument is ignored, for example OPENTELEMETRY_DEPRECATED_MESSAGE("name will be removed in the future") void Log(Severity severity, nostd::string_view name, nostd::string_view message) noexcept. We have discussed at #1383 before, and I think it's long enough to leave time for users to migrate now.

For users, we can still use the old Logger::Log APIs to emit logs, if we allow processor or exporters to use Recordable interface to send the logs without LogRecord, we can make Recordable derive from LogRecord. But if we changes or add more fields into LogRecord , we have to modify all Recordable implementations. Do you think it will be more difficult to maintain and upgrade for users?

Thanks for explanation. So to summarise

• We can remove all the deprecated Logger::Log() methods ( i.e. those containing name ), as we have given enough time for users to migrate.
• Rest of the existing methods can be (probably ?) renamed to Logger::EmitLog(), and they will keep using Recordable interface (which is derived from LogRecord class).

This looks fine to me. Log Data model is already stable, I don't think LogRecord would change any further, definitely not after log API is also made stable. So, there shouldn't be upgrade issues with users.

[owent]

intact

All APIs with the old name field are removed now.

Do you think we can keep the existing API intact (maybe renaming to Logger::EmitLog(..., ..., ...)) as mentioned in #1688. As per the logging API specs, the newly introduced interface to emit the log-records is only for Appenders. The specs doesn't enforce it for instrumentation library which makes sense because most of the languages (except C++) have logging api as part of language specs. The existing API can continue using direct Recordable interface to send the logs to exporter, without creating LogRecord object. Sometime, instrumentation library author would prefer to write a single line of code to emit logs.
And then the new interface can build the Recordable through ReadWriteLogRecord as you are doing right now in this PR.

Sorry, I'm poor in English and perhaps I didn't make it clear. The name field means All Logger::Log with name parameter, this argument is ignored, for example OPENTELEMETRY_DEPRECATED_MESSAGE("name will be removed in the future") void Log(Severity severity, nostd::string_view name, nostd::string_view message) noexcept. We have discussed at #1383 before, and I think it's long enough to leave time for users to migrate now.
For users, we can still use the old Logger::Log APIs to emit logs, if we allow processor or exporters to use Recordable interface to send the logs without LogRecord, we can make Recordable derive from LogRecord. But if we changes or add more fields into LogRecord , we have to modify all Recordable implementations. Do you think it will be more difficult to maintain and upgrade for users?

Thanks for explanation. So to summarise

• We can remove all the deprecated Logger::Log() methods ( i.e. those containing name ), as we have given enough time for users to migrate.
• Rest of the existing methods can be (probably ?) renamed to Logger::EmitLog(), and they will keep using Recordable interface (which is derived from LogRecord class).

This looks fine to me. Log Data model is already stable, I don't think LogRecord would change any further, definitely not after log API is also made stable. So, there shouldn't be upgrade issues with users.

Thanks, I will push another commit later to change the relationship between Recordable and LogRecord.

I also have a idea about the new Logger::EmitLog() APIs, how about using variable template when implement Logger::EmitLog(), and call specify LogRecord::SetXXX by type of argument?When we get trace::TraceId , call LogRecord::SetTraceId(),call LogRecord::SetAttribute() when we get any type that can be converted to common::KeyValueIterable and so on. So that we don't need to implement so many overload Logger::EmitLog() functions.

[lalitb]

I also have a idea about the new Logger::EmitLog() APIs, how about using variable template when implement Logger::EmitLog(), and call specify LogRecord::SetXXX by type of argument?When we get trace::TraceId , call LogRecord::SetTraceId(),call LogRecord::SetAttribute() when we get any type that can be converted to common::KeyValueIterable and so on. So that we don't need to implement so many overload Logger::EmitLog() functions.

Aren't variable templates part of C++14 ?

[owent]

I also have a idea about the new Logger::EmitLog() APIs, how about using variable template when implement Logger::EmitLog(), and call specify LogRecord::SetXXX by type of argument?When we get trace::TraceId , call LogRecord::SetTraceId(),call LogRecord::SetAttribute() when we get any type that can be converted to common::KeyValueIterable and so on. So that we don't need to implement so many overload Logger::EmitLog() functions.

Aren't variable templates part of C++14 ?

No, it's C++11. See https://en.cppreference.com/w/cpp/compiler_support/11

[lalitb]

I also have a idea about the new Logger::EmitLog() APIs, how about using variable template when implement Logger::EmitLog(), and call specify LogRecord::SetXXX by type of argument?When we get trace::TraceId , call LogRecord::SetTraceId(),call LogRecord::SetAttribute() when we get any type that can be converted to common::KeyValueIterable and so on. So that we don't need to implement so many overload Logger::EmitLog() functions.

Aren't variable templates part of C++14 ?

No, it's C++11. See https://en.cppreference.com/w/cpp/compiler_support/11

Oh variadic templates :) Not sure if that would look good at public API surface. I would prefer overloaded methods, but would leave it to you.

[owent]

@lalitb Could you please take some time to review this PR again when you have time?The Recordable is derived from LogRecord now.
I think I can rename the other Logger::Log into Logger::EmitLog in another PR and maybe we can also keep the old Log APIs for serveral versions to let users to migrate.

[lalitb]

Have some minor comments. In general looks in good shape. Need to review otlp exporter part to finish the review, will be doing that tomorrow.

[lalitb]

LGTM. with few minor comments.
We can wait for couple of days if there are more reviews before merging.

Thanks for the work, it is nicely done :)