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

chore: update logcall #15831

Merged
merged 6 commits into from
Jun 23, 2024
Merged

chore: update logcall #15831

merged 6 commits into from
Jun 23, 2024

Conversation

andylokandy
Copy link
Collaborator

@andylokandy andylokandy commented Jun 19, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-chore this PR only has small changes that no need to record, like coding styles. label Jun 19, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andylokandy)


src/meta/api/src/schema_api_impl.rs line 269 at r1 (raw file):

        req: CreateDatabaseReq,
    ) -> Result<CreateDatabaseReply, KVAppError> {
        debug!(req :? =(&req); "SchemaApi: {}", func_name!());

Is it able to keep the struct name SchemaApi in the debug log?

Copy link
Collaborator Author

@andylokandy andylokandy left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)


src/meta/api/src/schema_api_impl.rs line 269 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Is it able to keep the struct name SchemaApi in the debug log?

It's not possible in current logcall. But it's possible to upgrade.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andylokandy)


src/meta/api/src/schema_api_impl.rs line 269 at r1 (raw file):

Previously, andylokandy (Andy Lok) wrote…

It's not possible in current logcall. But it's possible to upgrade.

In that case, retaining the manual debug statements is a sensible choice. Given that there are several structures with nearly identical method sets, such as catalog.create_table invoking SchemaApi::create_table.

And having debug statements at the start of each method can be useful. This approach allows for better traceability and understanding of the workflow, especially when diagnosing issues, as opposed to only logging when a method completes.

@andylokandy
Copy link
Collaborator Author

having debug statements at the start of each method can be useful. This approach allows for better traceability and understanding of the workflow, especially when diagnosing issues, as opposed to only logging when a method completes.

For such aspect, I recommend enabling structlog for meta, which will arrange the log in the way functions is call in hierarchy, and is already setup for query

@andylokandy
Copy link
Collaborator Author

The new patch will log the function in full path: fast/logcall#5

@drmingdrmer
Copy link
Member

Can you keep the original debug log statement so that I can still get enough information while evaluating the new logging?

@andylokandy
Copy link
Collaborator Author

updated

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Thank you

Reviewed 2 of 2 files at r2, 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andylokandy)

@drmingdrmer drmingdrmer merged commit b10c3b2 into databendlabs:main Jun 23, 2024
73 of 74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-chore this PR only has small changes that no need to record, like coding styles.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants