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

add log of profile #16094

Merged
merged 2 commits into from
May 14, 2024
Merged

add log of profile #16094

merged 2 commits into from
May 14, 2024

Conversation

daviszhen
Copy link
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue ##15856

What this PR does / why we need it:

输出profile path 到日志里面。方便取profile文件。之前加的时候忘记了。

效果:

发送TERM信号:kill -15 mo_pid

收到TERM信号的结果:

{"level":"INFO","time":"2024/05/14 14:59:28.676438 +0800","caller":"mo-service/main.go:152","msg":"Starting shutdown...signal: terminated heap profile: ETL:/profile/heap_018f75e6-2601-75e0-8226-8f76b8384fed routine profile: ETL:/profile/routine_018f75e6-271d-79ff-a9c9-7eafe3eee054"}

发送INT信号:kill -2 mo_pid
收到INT 信号的结果:

{"level":"INFO","time":"2024/05/14 15:05:21.939141 +0800","caller":"mo-service/main.go:152","msg":"Starting shutdown...signal: interrupt. heap profile: ETL:/profile/heap_018f75eb-8ad4-78ab-9fc6-61cc7dac7f26 routine profile: ETL:/profile/routine_018f75eb-8b04-7eb5-a94a-e7644892ace1"}

没有profile的情况:
1, kill 信号。不会被拦截掉。不会有上面的日志。也不会有profile问题。
2,如果有上面的日志,但是etl里面没有profile文件。说明保存到etl中失败。可以通过下面的日志确认。

"save profile %s failed. err:%v

@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label May 14, 2024
@matrix-meow
Copy link
Contributor

@daviszhen Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating the addition of a log of profile.

Body:

The body of the pull request provides information about the type of PR, the related issue, and the purpose of the changes. It explains that the profile path is being output to the log for easier access to profile files. The body also includes examples of the expected log outputs for different scenarios.

Changes:

  1. Security Issue - Concatenation of User Input in Log Message:

    • The detail variable is being directly concatenated with profile paths without proper sanitization. This can lead to log injection vulnerabilities if the paths contain user-controlled data. To address this, use proper logging libraries that handle string concatenation securely or sanitize the profile paths before concatenating them.
  2. Code Duplication:

    • The code for saving heap and routine profiles is duplicated. Consider refactoring this code to a separate function to avoid redundancy and improve code maintainability.
  3. Missing Error Handling:

    • There is no error handling shown in the provided diff for the SaveProfile calls. It's important to handle potential errors that may occur during profile saving to prevent unexpected failures. Ensure proper error checking and logging for these operations.
  4. Inefficient String Concatenation:

    • The string concatenation in the detail variable can be optimized by using fmt.Sprintf or strings.Builder for better performance and readability.

Suggestions for Optimization:

  • Refactor the code to extract the duplicated logic for saving heap and routine profiles into a separate function to improve code readability and maintainability.
  • Implement proper error handling for the SaveProfile calls to handle any potential failures during profile saving.
  • Consider using a logging library that supports secure string concatenation to prevent log injection vulnerabilities.
  • Optimize string concatenation by using fmt.Sprintf or strings.Builder for better performance.

By addressing the security issue, code duplication, error handling, and optimizing string concatenation, the pull request can be enhanced for better code quality and security.

@mergify mergify bot merged commit c6b036c into matrixorigin:main May 14, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement size/XS Denotes a PR that changes [1, 9] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants