Support for python native logging from python wrapper #5279

BlackJohnny · 2019-09-25T22:45:40Z

Fixes #5620

Added support for Python native logging within the Pulsar Client Python wrapper.

added a new Logger implementation that forwards the logging to python
updated the python module to allow setting the logger on a pulsar.Client via the ClientConfiguration object

Tested with tag/2.4.1

merlimat

The change LGTM, just one comment on making more efficient the checking for log level.

merlimat · 2019-09-25T22:56:01Z

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

+        Py_XDECREF(_pyLogger);
+    }
+
+    bool isEnabled(Level level) {


It might be overkill to get back into Python each time to check the log level. This will be called many times (eg. for the debug statements). is the logging level supposed to be changed frequently at runtime? Could we cache it in c++ land and only check on python every 1sec or so?

For example, we have a RateLimiter in the perf directory. We could try to reuse the same class to "limit" the number of times/s we're checking what's the current log level

Thank you for the suggestion! It seems to me that RateLimiter blocks the caller and I think that it's not the intended behavior. Please correct me if I am wrong.

Taking into account that in real life you would not want to change the level at run-time very often, I propose the following options:

Solution 1:

Initialize a class member currentLoggingLevel during the construction of the Logger, by calling the python's "logger.getEffectiveLevel()". Then never update this value. This would imply that whenever the python programmer wants to change the logging behavior he would have to recycle the Client object.

Solution 2:

Add an internal counter to the logger and only forward the call to python each Nth time Logger::isEnabled is called.

Thank you for the suggestion! It seems to me that RateLimiter blocks the caller and I think that it's not the intended behavior. Please correct me if I am wrong.

Oh, that's right. I confused with the Java RateLimiter class which also has tryAcquire() which is non-blocking.

Solution 1:

I think that should be fine to start with

Solution 2:

It might be tricky to understand "when" the change is taken into account

Another possibility could be to just update a timestamp of the last time the effective level was refreshed. If it was more than 1sec, then refresh it and update the timer. There should be no need of synchronization, since it doesn't have to be perfectly correct.

merlimat

👍

jiazhai · 2019-09-27T07:38:13Z

run integration tests

merlimat · 2019-09-27T17:52:00Z

run integration tests

BlackJohnny · 2019-09-28T08:43:56Z

May I please have some help or hints with the Integration Tests? To say the least, I am not familiar enough with the project to know how to start fixing this issue.

Thank you!

jiazhai · 2019-09-29T09:33:53Z

run integration tests

jiazhai · 2019-09-30T02:46:51Z

run integration tests

jiazhai · 2019-09-30T02:48:11Z

@BlackJohnny, above comments "run integration tests" is a trigger for the jenkins integration tests, github will handle it.

merlimat · 2019-10-07T16:03:49Z

run integration tests

jiazhai · 2019-10-09T07:43:12Z

run integration tests

sijie · 2019-10-24T09:16:00Z

run integration tests

jiazhai · 2019-11-06T08:46:19Z

run integration tests

merlimat · 2019-11-11T20:25:12Z

run integration tests

sijie · 2019-11-12T09:04:35Z

run integration tests

sijie · 2019-11-25T22:20:03Z

run integration tests

sijie · 2019-11-26T22:23:29Z

run integration tests

sijie · 2019-11-27T22:04:32Z

run integration tests

sijie · 2019-11-28T06:45:50Z

run integration tests

sijie · 2019-11-28T22:04:17Z

run integration tests

sijie · 2019-11-29T01:50:02Z

run integration tests

sijie · 2019-11-29T05:11:11Z

@BlackJohnny do you mind rebasing this pull request to latest master to include some of the test fixes? I can't do it since you are creating a pull request based on the master of your fork.

BlackJohnny · 2019-12-04T08:41:36Z

@BlackJohnny do you mind rebasing this pull request to latest master to include some of the test fixes? I can't do it since you are creating a pull request based on the master of your fork.

Done. Is it moved to 2.6 because of my late reaction?

sijie · 2019-12-04T21:29:30Z

@BlackJohnny the community is currently deploying a time based release schedule. so I move the issue to 2.6.0 since I wasn't sure when this pull request will be taken care of as this pull request was made of your master branch and I can't change it.

jiazhai · 2019-12-17T12:01:29Z

retest this please

jiazhai · 2019-12-18T01:51:25Z

retest this please

BlackJohnny · 2020-01-20T10:38:24Z

@merlimat, @jiazhai What can I do to avoid the risk of not having this pull-request included in 2.6.0? The best solution should be the one that would trigger a merge sooner than the moment when 2.6.0 is about to be released. Please let me know what should I do to make this happen. Thanks!

sijie · 2020-01-20T19:30:47Z

@BlackJohnny Can you please rebase this pull request to latest master? Also it would be good to send the pull request from a non-master branch. The committers can help you rebase branch to latest master as needed.

BlackJohnny · 2020-01-21T08:34:42Z

Recreated pull-request from non-master branch Support for python native logging from python wrapper #6113

merlimat reviewed Sep 25, 2019

View reviewed changes

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Sep 25, 2019

merlimat added this to the 2.5.0 milestone Sep 25, 2019

merlimat approved these changes Sep 26, 2019

View reviewed changes

jiazhai approved these changes Sep 29, 2019

View reviewed changes

sijie assigned BlackJohnny Oct 24, 2019

merlimat mentioned this pull request Nov 11, 2019

cpp/python API: please log to stderr instead of stdout by default #5620

Closed

sijie modified the milestones: 2.5.0, 2.6.0 Dec 1, 2019

BlackJohnny closed this Jan 21, 2020

BlackJohnny force-pushed the master branch from 2a92460 to f8a7386 Compare January 21, 2020 08:29

BlackJohnny mentioned this pull request Jan 21, 2020

Support for python native logging from python wrapper #6113

Merged

Support for python native logging from python wrapper #5279

Support for python native logging from python wrapper #5279

Uh oh!

Conversation

BlackJohnny commented Sep 25, 2019 • edited by sijie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

merlimat Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

BlackJohnny Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

merlimat Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

jiazhai commented Sep 27, 2019

Uh oh!

merlimat commented Sep 27, 2019

Uh oh!

BlackJohnny commented Sep 28, 2019

Uh oh!

jiazhai commented Sep 29, 2019

Uh oh!

jiazhai commented Sep 30, 2019

Uh oh!

jiazhai commented Sep 30, 2019

Uh oh!

merlimat commented Oct 7, 2019

Uh oh!

jiazhai commented Oct 9, 2019

Uh oh!

sijie commented Oct 24, 2019

Uh oh!

jiazhai commented Nov 6, 2019

Uh oh!

merlimat commented Nov 11, 2019

Uh oh!

sijie commented Nov 12, 2019

Uh oh!

sijie commented Nov 25, 2019

Uh oh!

sijie commented Nov 26, 2019

Uh oh!

sijie commented Nov 27, 2019

Uh oh!

sijie commented Nov 28, 2019

Uh oh!

sijie commented Nov 28, 2019

Uh oh!

sijie commented Nov 29, 2019

Uh oh!

sijie commented Nov 29, 2019

Uh oh!

BlackJohnny commented Dec 4, 2019

Uh oh!

sijie commented Dec 4, 2019

Uh oh!

jiazhai commented Dec 17, 2019

Uh oh!

jiazhai commented Dec 18, 2019

Uh oh!

BlackJohnny commented Jan 20, 2020

Uh oh!

sijie commented Jan 20, 2020

Uh oh!

BlackJohnny commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

BlackJohnny commented Sep 25, 2019 •

edited by sijie

Loading