Logger with Admin Client #1758

pranavrth · 2024-06-03T11:46:02Z

Fixed logger not working when provided as an argument to AdminClient
Updated examples/adminapi.py to include usage of the custom logger with AdminClient

anchitj

Minor comments

anchitj · 2024-06-20T09:17:07Z

src/confluent_kafka/admin/__init__.py

        """
        Create a new AdminClient using the provided configuration dictionary.

        The AdminClient is a standard Kafka protocol client, supporting
        the standard librdkafka configuration properties as specified at
        https://github.com/edenhill/librdkafka/blob/master/CONFIGURATION.md

+        :param dict config: Configuration properties. At a minimum ``bootstrap.servers`` **should** be set\n"


Shouldn't it be named conf? And also the trailing \n can be removed

anchitj · 2024-06-20T09:20:30Z

src/confluent_kafka/admin/__init__.py

        """
        Create a new AdminClient using the provided configuration dictionary.

        The AdminClient is a standard Kafka protocol client, supporting
        the standard librdkafka configuration properties as specified at
        https://github.com/edenhill/librdkafka/blob/master/CONFIGURATION.md

+        :param dict config: Configuration properties. At a minimum ``bootstrap.servers`` **should** be set\n"
+        :param Logger logger: Optional Logger instance to use as a custom log messages handler.
+
        At least 'bootstrap.servers' should be configured.


This can be removed as it's included above now

anchitj · 2024-06-20T09:21:18Z

src/confluent_kafka/admin/__init__.py

        At least 'bootstrap.servers' should be configured.
        """
+        if logger is not None:


Do we need to throw an exception here if the conf doesn't includes bootstrap? like

if 'bootstrap.servers' not in conf: raise ValueError("Configuration must include 'bootstrap.servers'")

We are not doing this anywhere else. Let's not do here as well.

For anchit's comment about bootstrap.servers: this is checked later by librdkafka, whenever possible we delegate the validation there

anchitj · 2024-06-20T09:26:16Z

tests/test_log.py

+    stringBuffer = StringIO()
+    logger = logging.getLogger('Admin')
+    logger.setLevel(logging.DEBUG)
+    handler = logging.StreamHandler(stringBuffer)
+    handler.setFormatter(logging.Formatter('%(name)s Logger | %(message)s'))
+    logger.addHandler(handler)


This is being repeated here and the above two functions. Can be moved to setup_logger or some helper function

anchitj · 2024-06-20T09:27:05Z

examples/adminapi.py

@@ -920,3 +928,6 @@ def example_list_offsets(a, args):
        sys.exit(1)

    opsmap[operation](a, args)
+
+    # Log messages through custom logger if provided
+    a.poll(0)


Newline missing

anchitj · 2024-06-20T09:27:26Z

tests/test_log.py

+    logMessage = stringBuffer.getvalue().strip()
+    stringBuffer.close()
+
+    assert "Admin Logger | INIT" in logMessage


Newline missing

emasab · 2024-06-20T10:47:29Z

examples/adminapi.py

    # Create Admin client
-    a = AdminClient({'bootstrap.servers': broker})
+    a = AdminClient({'bootstrap.servers': broker},


logger should be a property like in the producer and consumer, not an additional parameter
'logger': logger,

In python, its an additional parameter as well.

right, I delete it so I don't request this change

* Updated examples/adminapi.py to include usage of the custom logger with AdminClient

emasab · 2024-06-20T11:07:33Z

src/confluent_kafka/admin/__init__.py

        At least 'bootstrap.servers' should be configured.
        """
+        if logger is not None:


For anchit's comment about bootstrap.servers: this is checked later by librdkafka, whenever possible we delegate the validation there

emasab · 2024-06-20T11:15:38Z

src/confluent_kafka/admin/__init__.py

        """
+        if logger is not None:
+            conf['logger'] = logger
        super(AdminClient, self).__init__(conf)


You can just pass the kwargs here without changing the conf, that'll be done by the C code

Suggested change

super(AdminClient, self).__init__(conf)

super(AdminClient, self).__init__(conf, **kwargs)

This doesn't work in case the logger is None. I tried this only first.

In general, if the logger is provided in conf as well as in the parameter, parameter gets more priority. By doing this, the logger is always None if it is provided in conf.

https://github.com/confluentinc/confluent-kafka-python/blob/master/src/confluent_kafka/src/confluent_kafka.c#L2232-L2241

Yes, was checking this an updated my comment with kwargs

emasab · 2024-06-20T11:25:23Z

src/confluent_kafka/admin/__init__.py

@@ -109,16 +109,19 @@ class AdminClient (_AdminClientImpl):
    Requires broker version v0.11.0.0 or later.
    """

-    def __init__(self, conf):
+    def __init__(self, conf, logger=None):


Suggested change

def __init__(self, conf, logger=None):

def __init__(self, conf, **kwargs):

This will start taking all the properties in the conf as parameter. I don't know if we should support this.

I was thinking of only supporting logger property like this.

That's what happens with the Producer and Consumer too

pranavrth · 2024-06-20T11:28:54Z

This is checked later by librdkafka, whenever possible we delegate the validation there

There is inbetween manipulation of the config properties in python binding as well. Due to that part, I have to check here first.

https://github.com/confluentinc/confluent-kafka-python/blob/master/src/confluent_kafka/src/confluent_kafka.c#L2215

pranavrth requested a review from a team as a code owner June 3, 2024 11:46

pranavrth mentioned this pull request Jun 3, 2024

AdminClient doesn't support specifying the logger #1699

Open

anchitj reviewed Jun 20, 2024

View reviewed changes

emasab requested changes Jun 20, 2024

View reviewed changes

pranavrth added 2 commits June 20, 2024 16:39

* Fixed logger not working when provided as an argument to AdminClient

d643392

* Updated examples/adminapi.py to include usage of the custom logger with AdminClient

PR comments

a2be27b

pranavrth force-pushed the dev_fix-logger-arg-admin-client branch from 5479623 to a2be27b Compare June 20, 2024 11:09

emasab requested changes Jun 20, 2024

View reviewed changes

pranavrth added 3 commits June 20, 2024 17:20

Fixed tests

df95432

PR comments

4faf35a

Added a separate logger example file for admin apis

53aac04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logger with Admin Client #1758

Logger with Admin Client #1758

pranavrth commented Jun 3, 2024

anchitj left a comment

anchitj Jun 20, 2024

anchitj Jun 20, 2024

anchitj Jun 20, 2024

pranavrth Jun 20, 2024

emasab Jun 20, 2024 •

edited

Loading

anchitj Jun 20, 2024

anchitj Jun 20, 2024

anchitj Jun 20, 2024

emasab Jun 20, 2024

pranavrth Jun 20, 2024

emasab Jun 20, 2024

emasab Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading

pranavrth Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading

pranavrth Jun 20, 2024

pranavrth Jun 20, 2024

emasab Jun 20, 2024

pranavrth commented Jun 20, 2024

	super(AdminClient, self).__init__(conf)
	super(AdminClient, self).__init__(conf, **kwargs)

	def __init__(self, conf, logger=None):
	def __init__(self, conf, **kwargs):

Logger with Admin Client #1758

Are you sure you want to change the base?

Logger with Admin Client #1758

Conversation

pranavrth commented Jun 3, 2024

anchitj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emasab Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emasab Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

emasab Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

pranavrth Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

emasab Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

emasab Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavrth commented Jun 20, 2024

emasab Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading

pranavrth Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading

emasab Jun 20, 2024 •

edited

Loading