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

Feature Improvement: Custom Key Names for Structured Logging Support #40251

Merged
merged 11 commits into from
Aug 31, 2022

Conversation

SadiHassan
Copy link
Contributor

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...
This is an extension of Structured logging feature which was merged in this PR #39277.
In this extension, user will be able to modify log key names. User will also be able to remove unwanted keys from log.
Consider this log below:
{"date_time":"1650918987.180175","thread_name":"#1","thread_id":"254545","level":"Trace","query_id":"","logger_name":"BaseDaemon","message":"Received signal 2","source_file":"../base/daemon/BaseDaemon.cpp; virtual void SignalListener::run()","source_line":"192"}.

To enable JSON logging support, user need to uncomment the entire <formatting> tag below taken from config.xml.

User will be able to modify key names by changing values under tag values inside tag. For example, to change date_time to MY_DATE_TIME, user can do like:
<date_time>MY_DATE_TIME</date_time>

User can stop unwanted log properties to appear in logs. To do so, they can simply comment out that property from this file.
For example, if you do not want your log to print query_id, you can comment out only <query_id> tag.However, if you comment out all the tags under , the program will print default values for as below.

    <!-- <formatting>
        <type>json</type>
        <names>
            <date_time>date_time</date_time>
            <thread_name>thread_name</thread_name>
            <thread_id>thread_id</thread_id>
            <level>level</level>
            <query_id>query_id</query_id>
            <logger_name>logger_name</logger_name>
            <message>message</message>
            <source_file>source_file</source_file>
            <source_line>source_line</source_line>
        </names>
    </formatting> -->

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@SadiHassan SadiHassan changed the title Structured-logging-custom-keys PR init Structured-logging-custom-keys Aug 16, 2022
@SadiHassan SadiHassan changed the title Structured-logging-custom-keys Feature Improvement: Custom Key Names for Structured Logging Support Aug 16, 2022
@robot-ch-test-poll robot-ch-test-poll added the pr-improvement Pull request with some product improvements label Aug 16, 2022
@evillique evillique self-assigned this Aug 16, 2022
@evillique evillique added the can be tested Allows running workflows for external contributors label Aug 16, 2022
Copy link
Member

@evillique evillique left a comment

Choose a reason for hiding this comment

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

It seems like although the .xml file in the test has changed we do not check whether the labels that we assigned are in the logs or not.

src/Loggers/OwnJSONPatternFormatter.h Outdated Show resolved Hide resolved
src/Loggers/OwnJSONPatternFormatter.cpp Outdated Show resolved Hide resolved
@SadiHassan
Copy link
Contributor Author

It seems like although the .xml file in the test has changed we do not check whether the labels that we assigned are in the logs or not.

Yes, we don't. Last time Alexey asked to make a simple test case that will only check if a valid JSON log is the output or not. However, I am open to add more checking if you require.

@SadiHassan
Copy link
Contributor Author

Hi @evillique , I updated the integration test code to cover different scenarios of custom keys. Please kindly have a look and give me feedback for any changes required. Thanks!

@SadiHassan
Copy link
Contributor Author

Hi @evillique , once you get a chance, can you please review my last updates and confirm?

Copy link
Member

@evillique evillique left a comment

Choose a reason for hiding this comment

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

Usually, tests with separate instances are made for testing communication between them, but the overhead does not seem to be large so it is probably ok.

src/Loggers/OwnJSONPatternFormatter.h Outdated Show resolved Hide resolved
src/Loggers/Loggers.cpp Outdated Show resolved Hide resolved
@SadiHassan SadiHassan force-pushed the Structured-logging-custom-keys branch from 1520009 to 87a28c2 Compare August 31, 2022 02:20
@evillique
Copy link
Member

Failed check seem unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants