refactor: log statement on error by Display (#3803)

* refactor: log statement on error by Display Signed-off-by: tison <[email protected]> * Apply suggestions from code review Co-authored-by: Yingwen <[email protected]> Signed-off-by: tison <[email protected]> --------- Signed-off-by: tison <[email protected]> Co-authored-by: Yingwen <[email protected]>
GreptimeTeam · Apr 26, 2024 · 4eadd9f · 4eadd9f
1 parent 1ec5951
commit 4eadd9f
Show file tree

Hide file tree

Showing 5 changed files with 8 additions and 68 deletions.
diff --git a/src/frontend/src/instance.rs b/src/frontend/src/instance.rs
@@ -315,20 +315,18 @@ impl SqlQueryHandler for Instance {
                         break;
                     }
 
-                    match self.query_statement(stmt, query_ctx.clone()).await {
+                    match self.query_statement(stmt.clone(), query_ctx.clone()).await {
                         Ok(output) => {
                             let output_result =
                                 query_interceptor.post_execute(output, query_ctx.clone());
                             results.push(output_result);
                         }
                         Err(e) => {
-                            let redacted = sql::util::redact_sql_secrets(query.as_ref());
                             if e.status_code().should_log_error() {
-                                error!(e; "Failed to execute query: {redacted}");
+                                error!(e; "Failed to execute query: {stmt}");
                             } else {
-                                debug!("Failed to execute query: {redacted}, {e}");
+                                debug!("Failed to execute query: {stmt}, {e}");
                             }
-
                             results.push(Err(e));
                             break;
                         }
@@ -448,9 +446,7 @@ impl PrometheusHandler for Instance {
             .execute_stmt(stmt, query_ctx.clone())
             .await
             .map_err(BoxedError::new)
-            .with_context(|_| ExecuteQuerySnafu {
-                query: format!("{query:?}"),
-            })?;
+            .context(ExecuteQuerySnafu)?;
 
         Ok(interceptor.post_execute(output, query_ctx)?)
     }

diff --git a/src/frontend/src/instance/prom_store.rs b/src/frontend/src/instance/prom_store.rs
@@ -149,9 +149,7 @@ impl Instance {
                 .handle_remote_query(&ctx, catalog_name, schema_name, &table_name, query)
                 .await
                 .map_err(BoxedError::new)
-                .with_context(|_| error::ExecuteQuerySnafu {
-                    query: format!("{query:#?}"),
-                })?;
+                .context(error::ExecuteQuerySnafu)?;
 
             results.push((table_name, output));
         }

diff --git a/src/servers/src/error.rs b/src/servers/src/error.rs
@@ -97,9 +97,8 @@ pub enum Error {
         error: Box<dyn std::error::Error + Send + Sync>,
     },
 
-    #[snafu(display("Failed to execute query, query: {}", query))]
+    #[snafu(display("Failed to execute query"))]
     ExecuteQuery {
-        query: String,
         location: Location,
         source: BoxedError,
     },

diff --git a/src/servers/src/query_handler/sql.rs b/src/servers/src/query_handler/sql.rs
@@ -84,10 +84,7 @@ where
             .do_query(query, query_ctx)
             .await
             .into_iter()
-            .map(|x| {
-                x.map_err(BoxedError::new)
-                    .context(error::ExecuteQuerySnafu { query })
-            })
+            .map(|x| x.map_err(BoxedError::new).context(error::ExecuteQuerySnafu))
             .collect()
     }
 
@@ -108,14 +105,7 @@ where
             .do_promql_query(query, query_ctx)
             .await
             .into_iter()
-            .map(|x| {
-                x.map_err(BoxedError::new).with_context(|_| {
-                    let query_literal = format!("{query:?}");
-                    error::ExecuteQuerySnafu {
-                        query: query_literal,
-                    }
-                })
-            })
+            .map(|x| x.map_err(BoxedError::new).context(error::ExecuteQuerySnafu))
             .collect()
     }
 

diff --git a/src/sql/src/util.rs b/src/sql/src/util.rs
@@ -13,20 +13,11 @@
 // limitations under the License.
 
 use std::fmt::{Display, Formatter};
-use std::sync::LazyLock;
 
-use regex::Regex;
 use sqlparser::ast::{Expr, ObjectName, SqlOption, Value};
 
 use crate::error::{InvalidTableOptionValueSnafu, Result};
 
-static SQL_SECRET_PATTERNS: LazyLock<Vec<Regex>> = LazyLock::new(|| {
-    vec![
-        Regex::new(r#"(?i)access_key_id=["']([^"']*)["'].*"#).unwrap(),
-        Regex::new(r#"(?i)secret_access_key=["']([^"']*)["'].*"#).unwrap(),
-    ]
-});
-
 /// Format an [ObjectName] without any quote of its idents.
 pub fn format_raw_object_name(name: &ObjectName) -> String {
     struct Inner<'a> {
@@ -59,37 +50,3 @@ pub fn parse_option_string(option: SqlOption) -> Result<(String, String)> {
     let k = key.value.to_lowercase();
     Ok((k, v))
 }
-
-/// Use regex to match and replace common seen secret values in SQL.
-pub fn redact_sql_secrets(sql: &str) -> String {
-    let mut s = sql.to_string();
-    for p in SQL_SECRET_PATTERNS.iter() {
-        if let Some(captures) = p.captures(&s) {
-            if let Some(m) = captures.get(1) {
-                s = s.replace(m.as_str(), "******");
-            }
-        }
-    }
-    s
-}
-
-#[cfg(test)]
-mod test {
-    use super::*;
-
-    #[test]
-    fn test_redact_sql_secrets() {
-        assert_eq!(
-            redact_sql_secrets(
-                r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='my_key_id', SECRET_ACCESS_KEY="my_access_key");"#
-            ),
-            r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='******', SECRET_ACCESS_KEY="******");"#
-        );
-        assert_eq!(
-            redact_sql_secrets(
-                r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='@scoped/key_id', SECRET_ACCESS_KEY="@scoped/access_key");"#
-            ),
-            r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='******', SECRET_ACCESS_KEY="******");"#
-        );
-    }
-}