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

The C API supports setting the log level. #158

Merged
merged 4 commits into from
Dec 29, 2022
Merged

Conversation

shibd
Copy link
Member

@shibd shibd commented Dec 27, 2022

Motivation

#137

Modifications

  • Add interface pulsar_client_configuration_set_logger_and_level to support set the log level.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@BewareMyPower
Copy link
Contributor

Not related to this PR, I found the .c file is not formatted by clang-format. I think you can fix the code style first. The clang-format check could be fixed in another PR.

@BewareMyPower BewareMyPower added the enhancement New feature or request label Dec 27, 2022
@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Dec 27, 2022
@shibd
Copy link
Member Author

shibd commented Dec 27, 2022

Not related to this PR, I found the .c file is not formatted by clang-format. I think you can fix the code style first. The clang-format check could be fixed in another PR.

Thanks for the reminder. I open a new PR #159 fix it.

@shibd shibd requested a review from BewareMyPower December 27, 2022 13:25
@shibd shibd requested review from RobertIndie and removed request for BewareMyPower December 28, 2022 08:38
@shibd shibd requested a review from BewareMyPower December 29, 2022 03:10
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

I prefer to change the definition of pulsar_logger and use the new function to deprecate the old function to set the logger:

// NOTE: you need to include <stdbool.h> to have bool in C
typedef struct pulsar_logger_t {
    void (*log)(pulsar_logger_level_t level, const char *file, int line, const char *message, void *ctx);

    bool (*is_enabled)(pulsar_logger_level_t level);
} pulsar_logger_t;

/**
 * Deprecated. Use `pulsar_client_configuration_set_logger_t` instead.
 */
PULSAR_PUBLIC void pulsar_client_configuration_set_logger(pulsar_client_configuration_t *conf,
                                                          pulsar_logger logger, void *ctx);

PULSAR_PUBLIC void pulsar_client_configuration_set_logger_t(pulsar_client_configuration_t *conf,
                                                            pulsar_logger_t logger, void *ctx);

This abstraction will be more like the C++'s Logger.

@BewareMyPower
Copy link
Contributor

The design might not be good, I think how to configure the logger might need more considerations. It's better to make the configuration more flexible.

@BewareMyPower
Copy link
Contributor

I will merge this PR for now. It's only included in the next 3.2.0 release. If I have a better idea, I will open a PR to remove the new API in this PR.

@BewareMyPower BewareMyPower merged commit f3fc502 into apache:main Dec 29, 2022
BewareMyPower added a commit to BewareMyPower/pulsar-client-cpp that referenced this pull request Dec 29, 2022
### Motivation

The current `pulsar_client_configuration_set_logger` API can only
configure the `Logger::log` method, but the` Logger::isEnabled` method
cannot be configured via C API.

apache#158 added a
`pulsar_client_configuration_set_logger_and_level` function to configure
a log level, but it's not flexible. For example, the log level might be
modified dynamically (though it's a complicated case).

### Modifications

Add a `pulsar_logger_t` struct and the related
`pulsar_client_configuration_set_logger_t` function to configure it as
the C logger API. The `is_enabled` and `log` fields of the struct are
the responding methods of the `isEnabled` and `log` methods in C++
`Logger`.

Then add a `LogContext` example in `SampleCustomLoggerCApi.c` to print
logs to a file or standard output.

Eliminate the `pulsar_client_configuration_set_logger_and_level`
function.
BewareMyPower added a commit to BewareMyPower/pulsar-client-cpp that referenced this pull request Dec 29, 2022
### Motivation

The current `pulsar_client_configuration_set_logger` API can only
configure the `Logger::log` method, but the` Logger::isEnabled` method
cannot be configured via C API.

apache#158 added a
`pulsar_client_configuration_set_logger_and_level` function to configure
a log level, but it's not flexible. For example, the log level might be
modified dynamically (though it's a complicated case).

### Modifications

Add a `pulsar_logger_t` struct and the related
`pulsar_client_configuration_set_logger_t` function to configure it as
the C logger API. The `is_enabled` and `log` fields of the struct are
the responding methods of the `isEnabled` and `log` methods in C++
`Logger`.

Then add a `LogContext` example in `SampleCustomLoggerCApi.c` to print
logs to a file or standard output.

Eliminate the `pulsar_client_configuration_set_logger_and_level`
function.
BewareMyPower added a commit that referenced this pull request Jan 4, 2023
### Motivation

The current `pulsar_client_configuration_set_logger` API can only
configure the `Logger::log` method, but the` Logger::isEnabled` method
cannot be configured via C API.

#158 added a
`pulsar_client_configuration_set_logger_and_level` function to configure
a log level, but it's not flexible. For example, the log level might be
modified dynamically (though it's a complicated case).

### Modifications

Add a `pulsar_logger_t` struct and the related
`pulsar_client_configuration_set_logger_t` function to configure it as
the C logger API. The `is_enabled` and `log` fields of the struct are
the responding methods of the `isEnabled` and `log` methods in C++
`Logger`.

Then add a `LogContext` example in `SampleCustomLoggerCApi.c` to print
logs to a file or standard output.

Eliminate the `pulsar_client_configuration_set_logger_and_level`
function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants