Added a `Plugin` trait #536

mdashti · 2023-08-02T20:23:54Z

This PR applies the following improvements:

As Address implements the std::fmt::Display trait, we'd better use {} formatter instead of {:?} to make the logs more readable.
There were some duplicate error messages, which should be distinguished for easier diagnosis. This is fixed in two instances.
The logs should indicate whether a plugin is enabled, instead of showing whether the plugin exists in the configurations. This is fixed.

levkk · 2023-08-02T20:49:04Z

Looks great! Just going to tag @drdrsh for the address logging change, since I remember him parsing the { .. } debug notation in their log processor to extract some metrics.

mdashti · 2023-08-04T10:19:56Z

Thanks @levkk. Then, I'll wait on @drdrsh's feedback.

drdrsh · 2023-08-04T21:55:43Z

@mdashti thank you for the PR. Let me check how the output looks.

drdrsh · 2023-08-07T14:38:48Z

The change to how Address objects is pretty significant. It will probably break how we parse the logs.

I think for logging we need two modes

A JSON mode with a high volume of information per line which is designed for generating metrics and semi-automated analysis.
A human-readable mode for humans.

and we can control this using a startup flag. What do you guys think?

mdashti · 2023-08-08T19:30:15Z

@drdrsh I think it makes sense. I can revert the Address-related changes in this PR for now, how's that?

mdashti · 2023-08-11T17:01:13Z

@drdrsh Reverted the Address logging changes in b9ff2be.

mdashti · 2023-09-22T08:35:13Z

Hi, any update on this PR?

mdashti · 2023-10-03T19:13:34Z

@levkk Is there specific changes needed for this PR? Currently, the "interceptor: {}, table_access: {}, query_logger: {}, prewarmer: {}" log line produces an incorrect result and this PR fixes it.

levkk · 2023-10-03T19:29:17Z

src/client.rs

@@ -531,7 +531,7 @@ where
                    error_response(
                        &mut write,
                        &format!(
-                            "No pool configured for database: {:?}, user: {:?}",
+                            "No pool configured for database: {:?}, user: {:?} (in startup)",


This is debug level messaging we should not be returning to the client.

Good point. Reverted this.

levkk · 2023-10-03T19:31:28Z

src/config.rs

@@ -763,15 +763,26 @@ pub struct Plugins {
    pub prewarmer: Option<Prewarmer>,
 }

+pub trait Plugin {


Nice to have a trait, but don't think you need it for this particular fix. We should discuss what trait we should have for a Plugin to implement, so we can get on our way to a real plugin system.

Second thought I see why you added it here.

I'm also thinking about the plugin system. I should write down my thoughts and share it with you. Please do the same if you have such notes.

levkk · 2023-10-03T19:32:40Z

src/client.rs

@@ -1354,7 +1354,7 @@ where
                    // Sync
                    // Frontend (client) is asking for the query result now.
                    'S' => {
-                        debug!("Sending query to server");
+                        debug!("Sending query to server  (in Sync mode)");


No such thing as "sync" mode, this is the extended protocol. Maybe make the debug line something like:

Sending query to server with extended protocol

Reverted this.

levkk · 2023-10-03T19:32:47Z

src/client.rs

@@ -1241,7 +1241,7 @@ where
                                let _ = query_router.infer(&ast);
                            }
                        }
-                        debug!("Sending query to server");
+                        debug!("Sending query to server (in Query mode)");


Same comment as below.

Reverted this.

levkk · 2023-10-03T19:37:01Z

src/config.rs

 impl std::fmt::Display for Plugins {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        fn is_enabled<T: Plugin>(arg: Option<&T>) -> bool {


Cleaner and safer to write it this way:

if let Some(arg) = arg { arg.is_enabled() } else { false }

That's a valid suggestion. Thanks. Done.

levkk

LGTM. Let me know if it's ready to merge, I'm happy to accept this as-is. Just going to let the CI run first.

mdashti

@levkk Thanks for the review.

mdashti · 2023-10-03T19:33:21Z

src/client.rs

@@ -531,7 +531,7 @@ where
                    error_response(
                        &mut write,
                        &format!(
-                            "No pool configured for database: {:?}, user: {:?}",
+                            "No pool configured for database: {:?}, user: {:?} (in startup)",


Good point. Reverted this.

mdashti · 2023-10-03T19:34:24Z

src/client.rs

@@ -1354,7 +1354,7 @@ where
                    // Sync
                    // Frontend (client) is asking for the query result now.
                    'S' => {
-                        debug!("Sending query to server");
+                        debug!("Sending query to server  (in Sync mode)");


Reverted this.

mdashti · 2023-10-03T19:39:26Z

src/config.rs

 impl std::fmt::Display for Plugins {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        fn is_enabled<T: Plugin>(arg: Option<&T>) -> bool {


That's a valid suggestion. Thanks. Done.

mdashti · 2023-10-03T19:40:00Z

src/client.rs

@@ -1241,7 +1241,7 @@ where
                                let _ = query_router.infer(&ast);
                            }
                        }
-                        debug!("Sending query to server");
+                        debug!("Sending query to server (in Query mode)");


Reverted this.

mdashti added 3 commits August 1, 2023 18:49

Improved logging

804fd0d

Improved logging for more Address usages

1424388

Fixed lint issues.

a1926fa

Reverted the Address logging changes.

b9ff2be

mdashti added a commit to crystalcld/pgcat that referenced this pull request Sep 22, 2023

Reverted the changes related to PR postgresml#536.

d3ffdea

mdashti changed the title ~~Logging improvements~~ Added a Plugin trait Sep 22, 2023

levkk reviewed Oct 3, 2023

View reviewed changes

Applied the PR comment by @levkk.

dcabbbb

levkk reviewed Oct 3, 2023

View reviewed changes

mdashti added 2 commits October 3, 2023 19:34

Applied the PR comment by @levkk.

2cb16d2

Applied the PR comment by @levkk.

dee386f

levkk reviewed Oct 3, 2023

View reviewed changes

Applied the PR comment by @levkk.

c23a5d1

levkk approved these changes Oct 3, 2023

View reviewed changes

mdashti commented Oct 3, 2023

View reviewed changes

levkk merged commit 3371c01 into postgresml:main Oct 3, 2023

mdashti deleted the improved-log branch October 4, 2023 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a `Plugin` trait #536

Added a `Plugin` trait #536

mdashti commented Aug 2, 2023

levkk commented Aug 2, 2023

mdashti commented Aug 4, 2023

drdrsh commented Aug 4, 2023

drdrsh commented Aug 7, 2023

mdashti commented Aug 8, 2023

mdashti commented Aug 11, 2023

mdashti commented Sep 22, 2023

mdashti commented Oct 3, 2023

levkk Oct 3, 2023

mdashti Oct 3, 2023

levkk Oct 3, 2023 •

edited

Loading

levkk Oct 3, 2023

mdashti Oct 3, 2023

levkk Oct 3, 2023

mdashti Oct 3, 2023

levkk Oct 3, 2023

mdashti Oct 3, 2023

levkk Oct 3, 2023 •

edited

Loading

mdashti Oct 3, 2023

levkk left a comment

mdashti left a comment

mdashti Oct 3, 2023

mdashti Oct 3, 2023

mdashti Oct 3, 2023

mdashti Oct 3, 2023

Added a Plugin trait #536

Added a Plugin trait #536

Conversation

mdashti commented Aug 2, 2023

levkk commented Aug 2, 2023

mdashti commented Aug 4, 2023

drdrsh commented Aug 4, 2023

drdrsh commented Aug 7, 2023

mdashti commented Aug 8, 2023

mdashti commented Aug 11, 2023

mdashti commented Sep 22, 2023

mdashti commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkk Oct 3, 2023 • 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

levkk Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkk left a comment

Choose a reason for hiding this comment

mdashti 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

Added a `Plugin` trait #536

Added a `Plugin` trait #536

levkk Oct 3, 2023 •

edited

Loading

levkk Oct 3, 2023 •

edited

Loading