Skip to content

feat(audit): 重构oracle top sql功能#3036

Merged
LordofAvernus merged 4 commits intomainfrom
fix_oracle_top_sql
May 12, 2025
Merged

feat(audit): 重构oracle top sql功能#3036
LordofAvernus merged 4 commits intomainfrom
fix_oracle_top_sql

Conversation

@Jarvis1105
Copy link
Copy Markdown
Contributor

@Jarvis1105 Jarvis1105 commented May 9, 2025

User description

关联的 issue

https://github.com/actiontech/sqle-ee/issues/2356

描述你的变更

  • 重构 DB 查询逻辑,支持按不同指标排序和筛选
  • 更新审计计划列表的排序选项,增加多个指标- 修改 Oracle Top SQL 任务的参数配置,简化设置
  • 为度量指标添加可排序标记,提升用户体验

确认项(pr提交后操作)

Tip

请在指定复审人之前,确认并完成以下事项,完成后✅


  • 我已完成自测
  • 我已记录完整日志方便进行诊断
  • 我已在关联的issue里补充了实现方案
  • 我已在关联的issue里补充了测试影响面
  • 我已确认了变更的兼容性,如果不兼容则在issue里标记 not_compatible
  • 我已确认了是否要更新文档,如果要更新则在issue里标记 need_update_doc


Description

  • 新增 collectIntervalMinute 参数支持

  • 优化 Top SQL 查询及资源释放

  • 更新指标查询与排序逻辑

  • 调整审计任务的排序标记


Changes walkthrough 📝

Relevant files
Enhancement
instance_audit_plan_list.go
更新 OrderByMap 字段映射规则                                                                         

sqle/model/instance_audit_plan_list.go

  • 修改 OrderByMap 字段映射
  • 添加新指标字段(query_time_total, cpu_time_total, 等)
+10/-5   
perf.go
调整 SQL 查询条件                                                                                           

sqle/pkg/oracle/perf.go

  • 增加 last_active_time 条件
  • 调整查询 SQL 语句条件
+1/-0     
task_type_oracle_topsql.go
更新审计任务参数配置                                                                                             

sqle/server/auditplan/task_type_oracle_topsql.go

  • 修改 Params 参数配置
  • 更新 QueryTopSQLs 函数调用参数
  • 为指标添加 Sortable 标记
+19/-26 
Bug fix
db.go
优化 QueryTopSQLs 查询函数                                                                         

sqle/pkg/oracle/db.go

  • 新增 collectIntervalMinute 参数
  • 用匿名函数优化查询处理
  • 改进错误处理及资源释放
+38/-17 

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • - 重构 DB 查询逻辑,支持按不同指标排序和筛选
    - 更新审计计划列表的排序选项,增加多个指标- 修改 Oracle Top SQL 任务的参数配置,简化设置
    -为度量指标添加可排序标记,提升用户体验
    @github-actions
    Copy link
    Copy Markdown

    github-actions Bot commented May 9, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 9f300da)

    🎫 Ticket compliance analysis 🔶

    2356 - Partially compliant

    Compliant requirements:

    • 重构 DB 查询逻辑
    • 更新排序选项
    • 修改任务参数配置
    • 添加指标的排序支持

    Non-compliant requirements:

    []

    Requires further human verification:

    []

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    参数不匹配

    在 Params 方法中,移除了 "top_n" 与 "order_by_column" 参数,但在 ExtractSQL 函数中仍通过 ap.Params 获取这两个参数,需确认是否为预期修改或补充默认值。

    	return nil, fmt.Errorf("get blacklist fail, error: %v", err)
    }
    // convert to string slice
    notInUser := make([]string, 0, len(dbUserBlacklists))
    for _, blacklist := range dbUserBlacklists {
    	notInUser = append(notInUser, blacklist.FilterContent)
    }
    // filter db user by
    sqls, err := db.QueryTopSQLs(ctx, ap.Params.GetParam("collect_interval_minute").String(), ap.Params.GetParam("top_n").Int(), notInUser, ap.Params.GetParam("order_by_column").String())
    if err != nil {
    	return nil, fmt.Errorf("query top sql fail, error: %v", err)
    逻辑确认

    在 QueryTopSQLs 方法中,当传入 orderBy 参数时,会覆盖预设的多个指标,仅使用传入的指标。请确认这种覆盖逻辑是否符合业务需求。

    metrics := []string{DynPerformanceViewSQLAreaColumnElapsedTime, DynPerformanceViewSQLAreaColumnCPUTime, DynPerformanceViewSQLAreaColumnDiskReads, DynPerformanceViewSQLAreaColumnBufferGets}
    if orderBy != "" {
    	metrics = []string{orderBy}
    }
    if topN == 0 {
    	topN = 10
    }

    @github-actions
    Copy link
    Copy Markdown

    github-actions Bot commented May 9, 2025

    PR Code Suggestions ✨

    Latest suggestions up to ce23fed

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    defer rows.Close usage

    建议在调用 o.db.QueryContext 后使用 defer rows.Close(),并移除错误分支中的 rows.Close() 调用,以防止在查询失败时对
    nil 调用 Close() 导致 panic。这样可以确保资源在正确的情况下释放。

    sqle/pkg/oracle/db.go [79-83]

     rows, err := o.db.QueryContext(ctx, query)
     if err != nil {
    -    rows.Close()
         return nil, errors.Wrapf(err, "failed to query %s", query)
     }
    +defer rows.Close()
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly addresses a potential panic by eliminating a premature call to rows.Close() in the error branch and deferring it after a successful call. This change improves error handling and resource management in the QueryTopSQLs function.

    Medium
    add missing parameters

    新增的 Params 函数只返回了 collect_interval_minute 参数,但 ExtractSQL 函数中却使用了 top_n
    order_by_column,建议补充这两个参数以避免运行时错误。请在 Params 返回值中添加 top_norder_by_column 参数定义。

    sqle/server/auditplan/task_type_oracle_topsql.go [34-43]

     func (at *OracleTopSQLTaskV2) Params(instanceId ...string) params.Params {
     	return []*params.Param{
     		{
     			Key:      paramKeyCollectIntervalMinute,
     			Value:    "60",
     			Type:     params.ParamTypeInt,
     			I18nDesc: locale.Bundle.LocalizeAll(locale.ParamCollectIntervalMinuteOracle),
     		},
    +		{
    +			Key:   "top_n",
    +			Desc:  "Top N",
    +			Value: "3",
    +			Type:  params.ParamTypeInt,
    +		},
    +		{
    +			Key:      "order_by_column",
    +			Value:    oracle.DynPerformanceViewSQLAreaColumnElapsedTime,
    +			Type:     params.ParamTypeString,
    +			I18nDesc: locale.Bundle.LocalizeAll(locale.ParamOrderByColumn),
    +		},
     	}
     }
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion highlights that the new Params function no longer provides the top_n and order_by_column parameters required by ExtractSQL, which could lead to runtime errors. Adding these parameters is necessary to maintain consistent functionality.

    Medium

    Previous suggestions

    Suggestions up to commit 8a32ed3
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    移除 nil 检查风险

    建议删除或修改在出错分支中对 rows.Close() 的调用,因为在发生错误时可能返回 nil,从而导致 nil 指针解引用的
    panic。可以在出错分支中直接返回错误,避免潜在的资源关闭问题。

    sqle/pkg/oracle/db.go [79-82]

     rows, err := o.db.QueryContext(ctx, query)
     if err != nil {
    -	rows.Close()
     	return nil, errors.Wrapf(err, "failed to query %s", query)
     }
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion effectively removes the risky call to rows.Close() when rows could be nil, which prevents a potential nil pointer dereference. This error handling improvement directly addresses a bug in the o.db.QueryContext logic.

    Medium
    Suggestions up to commit 05e37df
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Explicitly close resources

    将循环内对rows的关闭方式由defer
    rows.Close()
    改为在每次循环处理完毕后立即调用rows.Close(),以确保数据库连接能够及时关闭。此修改可以防止在循环中累积未关闭的行句柄,减少潜在的资源泄露风险。

    sqle/pkg/oracle/db.go [77-95]

     for _, metric := range metrics {
         query := fmt.Sprintf(DynPerformanceViewSQLAreaTpl, notInUsersStr, metric, topN)
         rows, err := o.db.QueryContext(ctx, query)
         if err != nil {
             return nil, errors.Wrapf(err, "failed to query %s", query)
         }
    -    defer rows.Close()
         for rows.Next() {
             res := DynPerformanceSQLArea{}
             err = rows.Scan(&res.SQLFullText, &res.Executions, &res.ElapsedTime, &res.UserIOWaitTime, &res.CPUTime, &res.DiskReads, &res.BufferGets, &res.UserName)
             if err != nil {
    +            rows.Close()
                 return nil, errors.Wrapf(err, "failed to scan %s", query)
             }
             ret = append(ret, &res)
         }
         if err := rows.Err(); err != nil {
    +        rows.Close()
             return nil, errors.Wrapf(err, "failed to iterate %s", query)
         }
    +    rows.Close()
     }
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that deferring rows.Close() within a loop can delay resource cleanup and potentially lead to resource exhaustion. By replacing it with an immediate call to rows.Close() after processing each query, the improvement is both relevant and accurately applied to the indicated code segment.

    Medium

    @github-actions
    Copy link
    Copy Markdown

    github-actions Bot commented May 9, 2025

    Persistent review updated to latest commit 8a32ed3

    Comment thread sqle/pkg/oracle/db.go Outdated
    @github-actions
    Copy link
    Copy Markdown

    Persistent review updated to latest commit ce23fed

    @github-actions
    Copy link
    Copy Markdown

    Persistent review updated to latest commit 9f300da

    @github-actions
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @LordofAvernus LordofAvernus merged commit bfec0d6 into main May 12, 2025
    4 checks passed
    @Jarvis1105 Jarvis1105 deleted the fix_oracle_top_sql branch May 12, 2025 07:37
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants