Uh oh!
There was an error while loading. Please reload this page.
feat(core): Add db.query.summary functionality - #21670
Conversation
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| return startSpanManual( | ||
| { | ||
| name: sanitizedSqlQuery || 'postgresjs.query', | ||
| name: querySummary || sanitizedSqlQuery || 'postgresjs.query', |
There was a problem hiding this comment.
cc @Lms24 this is what we want for span streaming, but I guess kind of breaking...? do we care...?
There was a problem hiding this comment.
Yeah, let's only add the attribute (this is a good thing IMO and can happen ASAP).
Let's hold off from making span name changes just yet. Talked about this with @cleptric when I worked on #21599. We'll very likely need to dynamcially decide what span name to set depending on span streaming enabled/disabled.
There was a problem hiding this comment.
Removed it from postgres. I still like to add this for #21656, though.
Lms24
left a comment
There was a problem hiding this comment.
See https://github.com/getsentry/sentry-javascript/pull/21670/changes#r3465674147
(Sorry in case my comment on Monday about this attribute being useful for span names prompted you to make the span name adjustments, I should have worded this more carefully. It will be very useful once we make the name change :D)
Uh oh!
There was an error while loading. Please reload this page.
| if (subSelect?.[1]) { | ||
| parts.push(subSelect[1]); | ||
| const selectTables = extractTableNames(rest.slice(subSelect.index)); | ||
| parts.push(...selectTables); |
There was a problem hiding this comment.
INSERT misreads SELECT in strings
Medium Severity
For INSERT statements, the follow-up scan uses a word-boundary SELECT match on the remainder of the query after the target table. That can match SELECT inside quoted string literals in VALUES clauses, so plain INSERT ... VALUES (...) queries may be summarized like INSERT ... SELECT ... with extra tokens from extractTableNames, skewing db.query.summary cardinality.
Reviewed by Cursor Bugbot for commit e94685f. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 34b5ca7. Configure here.
| return selectMatch.groups['operation']; | ||
| } | ||
| return truncate(query.trim().split(/\s+/)[0] ?? query); |
There was a problem hiding this comment.
Whitespace-only query mishandled
Low Severity
getSqlQuerySummary returns undefined for '' but not for strings that contain only whitespace. Those inputs fall through to the fallback, which can yield a non-empty whitespace string instead of undefined, unlike the empty-string case.
Reviewed by Cursor Bugbot for commit 34b5ca7. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
By implementing #21656 I saw that
db.query.summaryis not implemented. I wanted to use thenamein a low cardinality format, but there was no function for it. I therefore added the functionality for our postgres integration in order to havedb.query.summarywith a low cardinality format and also having the Span name in low cardinality.We are adding almost 800bytes in source code, but on the good side is that we send less bytes over the wire, which is IMO already a big win and a great justification for the bytes (also they are living "only" on the server so it is less tragic).
The OpenTelemetry docs only had few examples on how it should be done. Therefore the tests from the Java OpenTelemetry instrumentation were taken and put into sql.ported.test.ts in order to verify our function works as well. Unfortunately there are some edge cases which do not work just yet, because we use a regex implementation and wouldn't work with regex.
I also checked if the regex has some performance issues, but they should all be good.
Related
memejoke: https://programmerhumor.io/debugging-memes/the-plural-of-regex-is-regrets-jfzt