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

New LogRecord and Recordable implementations. #1766

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

owent
Copy link
Member

@owent owent commented Nov 13, 2022

Signed-off-by: WenTao Ou [email protected]

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.

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

@owent owent requested a review from a team November 13, 2022 14:12
@codecov
Copy link

codecov bot commented Nov 13, 2022

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

Impacted file tree graph

@@            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%) ⬇️

@owent owent changed the title [WIP] New LogRecord and Recordable implementations. New LogRecord and Recordable implementations. Nov 15, 2022
@owent owent force-pushed the update_logs_sdk branch 2 times, most recently from 740bf23 to 35f1ac8 Compare November 17, 2022 01:58
@lalitb
Copy link
Member

lalitb commented Nov 18, 2022

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
Copy link
Member Author

owent commented Nov 21, 2022

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
Copy link
Member

lalitb commented Nov 22, 2022

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
Copy link
Member Author

owent commented Nov 22, 2022

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
Copy link
Member

lalitb commented Nov 22, 2022

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
Copy link
Member Author

owent commented Nov 22, 2022

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
Copy link
Member

lalitb commented Nov 22, 2022

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 owent force-pushed the update_logs_sdk branch 4 times, most recently from e28b065 to 1bb5f7a Compare November 28, 2022 11:42
@owent
Copy link
Member Author

owent commented Nov 28, 2022

@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.

@owent owent force-pushed the update_logs_sdk branch 3 times, most recently from 043030a to dbe9b50 Compare December 4, 2022 10:28
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

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.

api/include/opentelemetry/logs/logger.h Show resolved Hide resolved
api/include/opentelemetry/logs/logger.h Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/processor.h Outdated Show resolved Hide resolved
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
sdk/src/logs/logger.cc Show resolved Hide resolved
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

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 :)

sdk/src/logs/readable_log_record.cc Show resolved Hide resolved
sdk/src/logs/read_write_log_record.cc Outdated Show resolved Hide resolved
…on#2941

Signed-off-by: owent <[email protected]>
Fix `-Werror=suggest-override` and style

Signed-off-by: owent <[email protected]>
Fix ostream_log_test

Signed-off-by: owent <[email protected]>
Fix ostream print_value for `AttributeValue`

Signed-off-by: owent <[email protected]>
New Recordable of logs

Signed-off-by: owent <[email protected]>
Restore .vscode/launch.json

Signed-off-by: owent <[email protected]>
Fix warning.

Signed-off-by: owent <[email protected]>
Fix warnings in maintainer mode and ETW exporter

Signed-off-by: owent <[email protected]>
Add CHANGELOG

Signed-off-by: owent <[email protected]>
Allow to move 'nostd::unique_ptr<T>' into `nostd::shared_ptr<T>`

Signed-off-by: owent <[email protected]>
Do not use `std/type_traits.h` any more. Maybe we should remove this file later.

Signed-off-by: owent <[email protected]>
Allow to add rvalue into `CircularBuffer`

Signed-off-by: owent <[email protected]>
Finish new `LogRecord` for exporters.

Signed-off-by: owent <[email protected]>
Finish unit tests in API and SDK.
Exporters are still work in progress.

Signed-off-by: owent <[email protected]>
New `LogRecord` and `Recordable` implementations.

Signed-off-by: WenTao Ou <[email protected]>

Restore `nostd::unique_ptr` to `std::unique_ptr` in sdk and exporters.

Signed-off-by: WenTao Ou <[email protected]>

Fix feedback of comments and useless headers

Signed-off-by: WenTao Ou <[email protected]>

Optimize if branch in `ReadWriteLogRecord::GetResource` .

Signed-off-by: owent <[email protected]>
@lalitb lalitb enabled auto-merge (squash) December 13, 2022 02:13
@lalitb lalitb merged commit 9f333ff into open-telemetry:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants