Skip to content

Sql insight ce #3095

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

Merged
merged 6 commits into from
Jul 28, 2025
Merged

Sql insight ce #3095

merged 6 commits into from
Jul 28, 2025

Conversation

BugsGuru
Copy link
Collaborator

@BugsGuru BugsGuru commented Jul 23, 2025

User description

关联的 issue

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

描述你的变更

  1. 修改工单SQL相关的execute_sql_detail表,增加SQL指纹字段,供性能洞察-关联SQL功能查询使用
  2. 新增sql_manage_raw_sqls表,使其保留扫描任务SQL详情,供性能洞察-关联SQL功能查询使用
  3. sql_manage_raw_sqls数据增加周期任务清理过期数据
  4. 调整MySQL processlist 扫描任务,保存原始SQL信息至sql_manage_raw_sqls,以便性能洞察-关联SQL功能查询具体单个SQL信息

assign in @iwanghc

确认项(pr提交后操作)

Tip

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


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


Description

  • 添加GetInstances接口便于实例查询

  • 新增系统变量及默认有效时长设置

  • 新增SQLManageRawSQL结构体与创建、清理函数

  • 更新ExecuteSQL结构体增加SqlFingerprint字段


Diagram Walkthrough

flowchart LR
  A["新增 GetInstances 接口"] --> B["新增系统变量与默认时长"]
  B --> C["新增 SQLManageRawSQL 结构体与方法"]
  C --> D["更新 ExecuteSQL 增加 SqlFingerprint"]
  D --> E["新增清理过期SQL原始记录功能"]
Loading

File Walkthrough

Relevant files

- Add SqlFingerprint field to ExecuteSQL struct
- Update audit logic to store SQL fingerprint
- Use longtext type for SqlFingerprint to accommodate large values
- Index SqlFingerprint for efficient querying
@actiontech-bot actiontech-bot requested a review from iwanghc July 23, 2025 08:44
Copy link

github-actions bot commented Jul 23, 2025

PR Code Suggestions ✨

Latest suggestions up to fbd2c59

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
实现数据库插入逻辑

实现 createSqlManageRawSQLs 函数以将传入的 SQL 数据写入数据库,而不是直接返回
nil。这样可以确保数据持久化并避免因空操作而导致潜在的数据不一致问题。请使用适当的数据库插入方法并检查错误。

sqle/model/sql_manage_insight_ce.go [6-8]

 func (s *Storage) createSqlManageRawSQLs(sqls []*SQLManageRawSQL) error {
+	if len(sqls) == 0 {
+		return nil
+	}
+	if err := s.db.Create(&sqls).Error; err != nil {
+		return err
+	}
 	return nil
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion replaces the stubbed implementation of createSqlManageRawSQLs with actual database insertion logic using s.db.Create, which is critical for data persistence. This improvement addresses a significant functional issue.

Medium
General
错误处理改进

json.Marshal 的错误进行处理,而不是将错误忽略。忽略错误可能导致错误数据写入数据库,难以发现潜在的数据问题。建议捕获并处理该错误后再构造返回值。

sqle/server/auditplan/sql_info.go [148-167]

 func ConvertSQLV2ToMangerRawSQL(sql *SQLV2) *model.SQLManageRawSQL {
-	data, _ := json.Marshal(sql.Info.ToMap()) // todo: 错误处理
+	data, err := json.Marshal(sql.Info.ToMap())
+	if err != nil {
+		// 根据实际需求,可选择打印日志或返回一个默认值
+		data = []byte("{}")
+	}
 	execTimeStr := sql.Info.Get(MetricNameLastReceiveTimestamp).String()
 	execTime, err := time.Parse(time.RFC3339, execTimeStr)
 	if err != nil {
 		execTime = time.Now()
 	}
 	return &model.SQLManageRawSQL{
 		Source:         sql.Source,
 		SourceId:       sql.SourceId,
 		ProjectId:      sql.ProjectId,
 		InstanceID:     sql.InstanceID,
 		SchemaName:     sql.SchemaName,
 		SqlFingerprint: sql.Fingerprint,
 		SqlText:        sql.SQLContent,
 		Info:           data,
 		SQLID:          sql.SQLId,
 		SqlExecTime:    execTime,
 	}
 }
Suggestion importance[1-10]: 8

__

Why: By handling errors from json.Marshal, the suggestion improves the robustness of the function ConvertSQLV2ToMangerRawSQL. This error handling change is important to avoid potential issues with incorrect or missing data processing.

Medium

Previous suggestions

Suggestions up to commit 5d446cd
CategorySuggestion                                                                                                                                    Impact
Possible issue
增加 json 错误处理

建议对 json.Marshal 的错误进行检测,而不是直接忽略。这样可以防止在数据转换失败时引发 panic 或异常,确保程序稳定性。

sqle/server/auditplan/sql_info.go [149]

-data, _ := json.Marshal(sql.Info.ToMap()) // todo: 错误处理
+data, err := json.Marshal(sql.Info.ToMap())
+if err != nil {
+    // 根据实际情况处理错误,例如记录日志或返回默认值
+    data = []byte("{}")
+}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies the ignored error from json.Marshal and proposes proper error handling, improving the stability of the conversion. The improvement is relevant and accurately reflects the change needed in the ConvertSQLV2ToMangerRawSQL function.

Medium
Suggestions up to commit 729aac0
CategorySuggestion                                                                                                                                    Impact
Possible issue
添加 json.Marshal 错误处理

建议在 ConvertSQLV2ToMangerRawSQL 函数中处理 json.Marshal
返回的错误,而不是忽略它。未处理的错误可能导致数据转换异常,从而引发潜在的系统错误。

sqle/server/auditplan/sql_info.go [149]

-data, _ := json.Marshal(sql.Info.ToMap()) // todo: 错误处理
+data, err := json.Marshal(sql.Info.ToMap())
+if err != nil {
+    // 记录错误并使用默认值,或者根据需要返回错误
+    data = []byte("{}")
+}
Suggestion importance[1-10]: 8

__

Why: The suggestion improves reliability by properly handling the error from json.Marshal in the ConvertSQLV2ToMangerRawSQL function, reducing potential runtime issues.

Medium
Suggestions up to commit 7bd06c1
CategorySuggestion                                                                                                                                    Impact
General
增加解析错误日志

建议在解析 sv.Value 为整数时增加日志记录或者更详细的错误提示信息,以便在解析失败时能够更清楚地定位问题。这样可以提高代码的可维护性,帮助快速发现数据异常情况。

sqle/model/configuration.go [135-141]

 if sv.Key == SystemVariableSqlManageQueueExpiredHours {
 	expiredHs, err := strconv.ParseInt(sv.Value, 10, 64)
 	if err != nil {
+		log.Errorf("解析系统变量 [%s] 值失败: %v", SystemVariableSqlManageQueueExpiredHours, err)
 		return 0, err
 	}
 	return expiredHs, nil
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion introduces an error log when parsing sv.Value fails, which aids in debugging. However, it is a minor logging improvement rather than fixing a core bug.

Low
添加时间解析错误日志

建议在时间解析失败时记录错误日志,而不仅仅是默默地使用当前时间,这样便于后续诊断数据问题。记录日志可以帮助及时发现并处理数据格式异常。

sqle/server/auditplan/sql_info.go [135-139]

 execTimeStr := sql.Info.Get(MetricNameLastReceiveTimestamp).String()
 execTime, err := time.Parse(time.RFC3339, execTimeStr)
 if err != nil {
+	log.Warnf("时间解析失败,使用当前时间代替, 原始时间字符串: %s, 错误: %v", execTimeStr, err)
 	execTime = time.Now()
 }
Suggestion importance[1-10]: 5

__

Why: This suggestion adds a warning log for time parsing errors, offering better traceability for data issues. It is a useful improvement but not critical for functionality.

Low
Suggestions up to commit 67b46d6
CategorySuggestion                                                                                                                                    Impact
General
增加错误处理日志

建议在时间解析失败时增加错误日志记录或注释说明,以便在出现解析失败时能够清楚知道发生了什么问题。这样有助于后续问题排查,避免隐藏数据不准确的问题。

sqle/server/auditplan/sql_info.go [135-139]

 execTimeStr := sql.Info.Get(MetricNameLastReceiveTimestamp).String()
 execTime, err := time.Parse(time.RFC3339, execTimeStr)
 if err != nil {
+	// 记录错误或做进一步处理,避免隐藏解析错误
 	execTime = time.Now()
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion improves error visibility by adding a comment for logging when time.Parse fails, which is useful for debugging but represents a minor improvement.

Low
Possible issue
检查SQL聚合逻辑

建议确认移除调用 AggregateSQL 的逻辑后,不会导致重复 SQL 数据无法正确去重或聚合。请验证新逻辑是否满足业务需求,否则可能会引入数据重复或遗漏风险。

sqle/server/auditplan/task_type_mysql_processlist.go [93-102]

 sqls := make([]*SQLV2, 0, len(res))
 for i := range res {
 	if at.filterFullProcessList(res[i], sqlMinSecond, db.Db.GetConnectionID()) {
 		continue
 	}
-	...
+	// 注意:确认是否需要新增去重或聚合处理逻辑
 	sqls = append(sqls, sqlV2)
 }
 return sqls, nil
Suggestion importance[1-10]: 5

__

Why: The suggestion prompts a verification of the removed AggregateSQL logic to avoid data duplication or loss, but it only adds a clarifying comment without changing the actual functionality.

Low

iwanghc
iwanghc previously approved these changes Jul 23, 2025
@iwanghc iwanghc merged commit 958e7e8 into main Jul 28, 2025
4 checks passed
@BugsGuru BugsGuru deleted the sql-insight-ce branch July 28, 2025 02:35
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