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

feat:(logging) Added Log Format provision #8890

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

damanV5
Copy link

@damanV5 damanV5 commented Feb 25, 2025

Added log_format feature for logger.

@damanV5
Copy link
Author

damanV5 commented Feb 25, 2025

@zachmu

@timsehn
Copy link
Contributor

timsehn commented Feb 25, 2025

So, all you've done is make the configuration valid in the configuration YAML file.

Now, you have to plumb it through to change the actual logging behavior.

And finally, you should write some tests to make sure it actually logs in JSON if you have the setting enabled.

@damanV5
Copy link
Author

damanV5 commented Feb 25, 2025

@timsehn Can you take a look whether I am heading in the right direction?

@timsehn
Copy link
Contributor

timsehn commented Feb 25, 2025

You are heading in the right direction. It looks like you have most of the places to actually get the value to the server. Now the server needs to read it when it wants to emit a log line and emit the right format.

@zachmu suggested that logic is here:

https://github.com/dolthub/dolt/blob/main/go/cmd/dolt/commands/sqlserver/server.go#L117

Looking at logrus (https://github.com/sirupsen/logrus), it seems they have JSON formatting capability. IN their README, they suggest

log.SetFormatter(&log.JSONFormatter{})

@damanV5
Copy link
Author

damanV5 commented Feb 26, 2025

@timsehn I have configured the logger for log_format but i am not sure how can I test the same with a logger.
So far, I am able to write a test-case to verify DefaultLogFormat.

Update:-
I have built the go folder locally and tested with log_format: json in the yaml. Following is the output:-

image

@timsehn
Copy link
Contributor

timsehn commented Feb 26, 2025

That looks like it is working. I'm not sure the best way to write tests for this. @zachmu should answer.

@zachmu
Copy link
Member

zachmu commented Feb 26, 2025

Hi @damanV5,

This looks really good. You should add a test for the new option in the bats file here, similar to this one:

https://github.com/dolthub/dolt/blob/main/integration-tests/bats/sql-server.bats#L123

You can check the output format is correct by redirecting the log to a file, like this test does:

https://github.com/dolthub/dolt/blob/main/integration-tests/bats/sql-server.bats#L1531

@lucasfcnunes
Copy link

@timsehn I have configured the logger for log_format but i am not sure how can I test the same with a logger.
So far, I am able to write a test-case to verify DefaultLogFormat.

Update:-
I have built the go folder locally and tested with log_format: json in the yaml. Following is the output:-

image

@timsehn @damanV5

Can this very first line too be in json?

@damanV5
Copy link
Author

damanV5 commented Feb 27, 2025

Need to figure something out here,

The following test is working fine for insensitivity when checked from command line but from config.yaml it is behaving sensitive. So, that I need to figure out.

Test:-

@test "sql-server: logformats are case insensitive" {
    # assert that logformat on command line is not case sensitive
    cd repo1
    PORT=$( definePORT )
    dolt sql-server --logformat jSon --port=$PORT --socket "dolt.$PORT.sock" > log.txt 2>&1 &
    SERVER_PID=$!
   wait_for_connection $PORT 8500 || { 
    echo "Server failed to start, dumping logs:"
    cat log.txt
    exit 1
}
    dolt sql -q "show databases;"
    stop_sql_server

    # assert that logformat in yaml config is not case sensitive
    cat >config.yml <<EOF
log_format: teXt
behavior:
  disable_client_multi_statements: true
listener:
  host: "0.0.0.0"
  port: $PORT
EOF
    dolt sql-server --config ./config.yml --socket "dolt.$PORT.sock" &
    SERVER_PID=$!
    wait_for_connection $PORT 8500
    dolt sql -q "show databases;"
    stop_sql_server
}

@damanV5
Copy link
Author

damanV5 commented Feb 27, 2025

@zachmu Update: I managed to add a test case for log_format.

@timsehn
Copy link
Contributor

timsehn commented Feb 27, 2025

Running tests now.

@@ -145,6 +145,32 @@ EOF
dolt sql -q "show databases;"
stop_sql_server
}
@test "sql-server: logformats are case insensitive" {
Copy link
Contributor

@timsehn timsehn Feb 27, 2025

Choose a reason for hiding this comment

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

You need a test here to assert that the log outputted in json format.

Copy link
Author

Choose a reason for hiding this comment

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

@timsehn Added.

@timsehn
Copy link
Contributor

timsehn commented Feb 27, 2025

Also, you must fix the Go tests this broke.

@timsehn
Copy link
Contributor

timsehn commented Feb 27, 2025

It's odd that the Go tests passed with No race but failed with race on.

@damanV5
Copy link
Author

damanV5 commented Feb 28, 2025

@timsehn I have fixed the go tests for sqlserver.

Do i also need to update minver_validation.txt with LogFormatStr *string 0.0.0 log_format,omitempty as the header says "file automatically updated by the release process". ?

@damanV5 damanV5 requested a review from timsehn February 28, 2025 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants