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

Add tracing subscriber to enable logging #78

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

Gor027
Copy link
Contributor

@Gor027 Gor027 commented Sep 16, 2022

Motivation:

Added tracing-subscriber crate to have one common way of logging with the Rust driver, as the Rust driver uses tracing crate for logging and cpp-rust driver should allow clients to configure logging by specifying the max log level and providing callback that should be called on each event.

Solution:

As logging should be configured globally, the current implementation creates a custom tracing subscriber with default configuration, which later can be changed by the user. The global and mutable state of the logging configuration is initialized in the lazy_static! block, however, it will be evaluated only after the first access in the code. As, the cass_session_new() function most probably is the first function that the user will call (almost all tests do that, too), the initialization of logging is chosen to be done in this function.

The CassLogMessage has a field function that should always be empty, as it cannot be properly set, because event metadata does not include information about it.

Enabled LoggingTests tests in github actions.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have enabled appropriate tests in .github/workflows/build.yaml in gtest_filter.

@Gor027 Gor027 force-pushed the add_logging branch 4 times, most recently from 995275e to 5a9d7c0 Compare September 20, 2022 13:22
@Gor027 Gor027 mentioned this pull request Sep 20, 2022
4 tasks
@Gor027 Gor027 force-pushed the add_logging branch 2 times, most recently from 8beac0e to 1a66462 Compare September 21, 2022 08:55
scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/argconv.rs Show resolved Hide resolved
scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/lib.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/lib.rs Outdated Show resolved Hide resolved
@Gor027 Gor027 mentioned this pull request Oct 2, 2022
10 tasks
scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Show resolved Hide resolved
src/logger.cpp Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Show resolved Hide resolved
@Gor027 Gor027 force-pushed the add_logging branch 2 times, most recently from 23c76c5 to 8a3e35f Compare November 22, 2022 11:01
@Gor027 Gor027 requested a review from Lorak-mmk November 22, 2022 23:08
scylla-rust-wrapper/src/cass_log.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/lib.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
@Gor027 Gor027 force-pushed the add_logging branch 2 times, most recently from f81fa7c to b7c1ae0 Compare December 10, 2022 12:00
@Gor027 Gor027 requested a review from Lorak-mmk December 12, 2022 12:20
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
CassLogLevel::CASS_LOG_ERROR => Level::ERROR,
CassLogLevel::CASS_LOG_WARN => Level::WARN,
CassLogLevel::CASS_LOG_INFO => Level::INFO,
CassLogLevel::CASS_LOG_DEBUG => Level::DEBUG,
CassLogLevel::CASS_LOG_TRACE => Level::TRACE,
CassLogLevel::CASS_LOG_CRITICAL => Level::ERROR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here - either put critical on top or reverse the order of other fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/logging.rs Outdated Show resolved Hide resolved
Motivation:

Added `tracing-subscriber` crate to have one common way of logging with
the Rust driver, as the Rust driver uses `tracing` crate for logging and
cpp-rust driver should allow clients to configure logging by specifying
the max log level and providing callback that should be called on each
event.

Solution:

As logging should be configured globally, the current implementation
creates a custom tracing subscriber with default configuration, which
later can be changed by the user. The global and mutable state of the
logging configuration is initialized in the `lazy_static!` block,
however, it will be evaluated only after the first access in the code.
As, the `cass_session_new()` function most probably is the first
function that the user will call (almost all tests do that, too), the
initialization of logging is chosen to be done in this function.

The CassLogMessage has a field `function` that should always be empty,
as it cannot be properly set, because event metadata does not include
information about it.

Enabled `LoggingTests` tests in github actions.
@Lorak-mmk Lorak-mmk merged commit 0d2a2d1 into scylladb:master Dec 19, 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
Development

Successfully merging this pull request may close these issues.

4 participants