implement python logger wrapper #7713

lbenc135 · 2020-07-31T18:24:08Z

Motivation

Logging in the python client is not configurable, so the stdout was spammed. This PR revives the #6113 but tries to resolve the bugs that resulted the last time.

Modifications

Implemented a python logger wrapper. You can pass a python logger as an argument to the client.

Verifying this change

Make sure that the change passes the CI checks.

Not sure how to unit-test this. I tested it manually by building the client and trying it out with python scripts. It works as expected both with the logger forwarded and without. Also, configuring the logger also configures the log level so this fixes the problem.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): no
The public API: yes (?)
The schema: no
The default values of configurations: no
The wire protocol: no
The rest endpoints: no
The admin cli options: no
Anything that affects deployment: no

Documentation

Does this pull request introduce a new feature? yes
If yes, how is the feature documented? inline function docs

pinkavaj

Looks good. Mey be add at least some simple test, if it is managable?

pulsar-client-cpp/python/src/config.cc

lbenc135 · 2020-12-28T08:31:57Z

@pinkavaj could you take another look please?

pinkavaj · 2021-01-21T20:02:32Z

pulsar-client-cpp/python/src/config.cc

+        return _getLogLevelValue(level) >= _currentPythonLogLevel;
+    }
+
+    void log(Level level, int line, const std::string& message) {


The file and line information is not logged, but I'm not familiar with the code enought to decide what is more appropriate. From the point of developer I would preffer the file and line information in the log message.

Probably someone more familiar with this code should speek his opinion.

Otherwise looks good.

Feel free to show example and you incentive to help with decission.

Not having this info only truly matters if the Python Logger is connected ultimately to one or more Formatters that has tokens like filename, pathname or lineno in its format string.

It should be possible to create a LogRecord object (https://docs.python.org/3/library/logging.html#logrecord-objects) and pass it to Logger.handle method instead of using the simpler info, warning etc methods. I for one think it is not worth the trouble and the implementation is fine as-is.

Thank you @Bklyn, please consider approving the PR

pulsar-client-cpp/python/pulsar/__init__.py

pulsar-client-cpp/python/src/config.cc

Bklyn

Looks good to be with the small exception of not initializing the _logger string member in the copy-ctor and assignment operator, but is this member even used for anything? I might suggest dropping it entirely.

Bklyn

Looks good to me!

aahmed-se · 2021-03-25T14:38:43Z

rebase this on master.

tuteng · 2021-03-25T23:30:51Z

/pulsarbot run-failure-checks

tuteng · 2021-03-27T03:06:33Z

/pulsarbot run-failure-checks

mianos · 2021-04-22T07:20:18Z

Does anyone know the why the unit test run was cancelled?
This PR would be great to get into our unloved python client.

Giaco9 · 2021-05-20T09:01:56Z

Hi everyone, thank you for the effort put in this pull request! Would be possible to release a version compatible with python2.7 that includes these changes?

Thank you!

Fixes apache#4234 ### Motivation Logging in the python client is not configurable, so the stdout was spammed. This PR revives the apache#6113 but tries to resolve the bugs that resulted the last time. ### Modifications Implemented a python logger wrapper. You can pass a python logger as an argument to the client. ### Verifying this change - [ ] Make sure that the change passes the CI checks. Not sure how to unit-test this. I tested it manually by building the client and trying it out with python scripts. It works as expected both with the logger forwarded and without. Also, configuring the logger also configures the log level so this fixes the problem.

mianos · 2021-06-30T01:18:25Z

Now this is merged I can't seem to get it working. I suspect it is because this logger is not loaded in the C++.
I would expect the massive number of INFO messages would be suppressed by doing the following:

    plogger = logging.getLogger("pulsar")
    print(f"current level {plogger.getEffectiveLevel()}")
    plogger.info("Hello info")
    plogger.setLevel(logging.ERROR)
    print(f"post setting current level {plogger.getEffectiveLevel()}")
    plogger.info("post unset info Hello info")
    client = pulsar.Client(pulsar_config.uri, logger=plogger)

I see the normal python logger suppresses the info messages but the prefect-client 2.8 still prints all
the INFO messages.
Maybe the unit test should set a logger handler and check it's not full of INFO messages?

Fixes apache#4234 ### Motivation Logging in the python client is not configurable, so the stdout was spammed. This PR revives the apache#6113 but tries to resolve the bugs that resulted the last time. ### Modifications Implemented a python logger wrapper. You can pass a python logger as an argument to the client. ### Verifying this change - [ ] Make sure that the change passes the CI checks. Not sure how to unit-test this. I tested it manually by building the client and trying it out with python scripts. It works as expected both with the logger forwarded and without. Also, configuring the logger also configures the log level so this fixes the problem.

lbenc135 mentioned this pull request Jul 31, 2020

Fix Log4cxx support #7662

Closed

1 task

lbenc135 force-pushed the feature/python_logger branch from 5f17396 to deebbe8 Compare September 5, 2020 08:38

pinkavaj reviewed Sep 18, 2020

View reviewed changes

pulsar-client-cpp/python/src/config.cc Outdated Show resolved Hide resolved

lbenc135 force-pushed the feature/python_logger branch from 0c09e10 to 6fcf18f Compare October 3, 2020 07:48

lbenc135 force-pushed the feature/python_logger branch from 9a16be7 to 11be0b3 Compare December 28, 2020 08:34

pinkavaj reviewed Jan 21, 2021

View reviewed changes

lbenc135 force-pushed the feature/python_logger branch from 11be0b3 to 54ebbdb Compare March 20, 2021 08:08

Bklyn reviewed Mar 22, 2021

View reviewed changes

pulsar-client-cpp/python/pulsar/__init__.py Outdated Show resolved Hide resolved

Bklyn reviewed Mar 22, 2021

View reviewed changes

pulsar-client-cpp/python/src/config.cc Show resolved Hide resolved

Bklyn reviewed Mar 22, 2021

View reviewed changes

pulsar-client-cpp/python/src/config.cc Show resolved Hide resolved

Bklyn suggested changes Mar 22, 2021

View reviewed changes

lbenc135 force-pushed the feature/python_logger branch from f49f281 to 7048f59 Compare March 23, 2021 10:46

Bklyn approved these changes Mar 23, 2021

View reviewed changes

livio and others added 8 commits March 25, 2021 15:40

implement python logger wrapper

fe775af

rerun tests

838d6ae

revert accidental merge change

ffe130b

extract log level equation to a function

2aa9c02

add python logger test

4cb8627

try to fix the license error

2085231

update documentation

2dd8f74

remove redundant code

e82dab9

lbenc135 force-pushed the feature/python_logger branch from 7048f59 to e82dab9 Compare March 25, 2021 14:40

tuteng approved these changes Mar 25, 2021

View reviewed changes

tuteng assigned lbenc135 Mar 25, 2021

tuteng added component/c++ type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Mar 25, 2021

tuteng added the area/client label Mar 25, 2021

tuteng added this to the 2.8.0 milestone Mar 25, 2021

merlimat approved these changes Mar 27, 2021

View reviewed changes

Merge branch 'master' into feature/python_logger

3da66f1

codelipenghui merged commit b6247fd into apache:master May 20, 2021

lbenc135 deleted the feature/python_logger branch May 20, 2021 09:12

BewareMyPower mentioned this pull request Jun 15, 2021

(2.7.2) Python client blocks forever in "create_producer" during a broker failure, even if "operation_timeout_seconds" is set #10721

Closed

Samreay mentioned this pull request Jan 15, 2024

Custom logger doesn't work with created resources (like producers) apache/pulsar-client-python#194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement python logger wrapper #7713

implement python logger wrapper #7713

lbenc135 commented Jul 31, 2020

pinkavaj left a comment

lbenc135 commented Dec 28, 2020

pinkavaj Jan 21, 2021

pinkavaj Jan 21, 2021

Bklyn Mar 16, 2021

lbenc135 Mar 20, 2021

Bklyn left a comment

Bklyn left a comment

aahmed-se commented Mar 25, 2021

tuteng commented Mar 25, 2021

tuteng commented Mar 27, 2021

mianos commented Apr 22, 2021

Giaco9 commented May 20, 2021

mianos commented Jun 30, 2021

implement python logger wrapper #7713

implement python logger wrapper #7713

Conversation

lbenc135 commented Jul 31, 2020

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

pinkavaj left a comment

Choose a reason for hiding this comment

lbenc135 commented Dec 28, 2020

pinkavaj Jan 21, 2021

Choose a reason for hiding this comment

pinkavaj Jan 21, 2021

Choose a reason for hiding this comment

Bklyn Mar 16, 2021

Choose a reason for hiding this comment

lbenc135 Mar 20, 2021

Choose a reason for hiding this comment

Bklyn left a comment

Choose a reason for hiding this comment

Bklyn left a comment

Choose a reason for hiding this comment

aahmed-se commented Mar 25, 2021

tuteng commented Mar 25, 2021

tuteng commented Mar 27, 2021

mianos commented Apr 22, 2021

Giaco9 commented May 20, 2021

mianos commented Jun 30, 2021