Apm: SQL Parsing reference implementation

[alvarolobato]

We've been discussing the SQL parsing again and will go for a programming language specific approach, meaning that all the agents will have to create their own implementation.

To make it easier we will initially create a reference implementation in the Go agent together with a JSON test suite.

Other agents will follow doing their own implementation as close as language differences permit to the reference implementation and use the test definitions to check their implementation.

This issue will track the creation of the reference implementation in the Go agent and the test cases.

[axw]

The Go agent has a lenient scanner (https://github.com/elastic/apm-agent-go/tree/master/internal/sqlscanner) and parser (https://github.com/elastic/apm-agent-go/blob/master/internal/sqlutil/signature.go) for creating a short transaction name.

The test cases need to be extracted as something reusable.

I'll document my assumptions here later, so we can ensure the approach is sound before we all jump on it.

[axw]

Requirements

• Zero-to-low memory allocation, low CPU overhead. DB operations are frequently on hot code paths, so we must minimize overhead.
• Parsing should gracefully handle unexpected SQL. We can't hope to keep up with the myriad SQL dialects out there. Instead, handle the most common parts of the most common dialects, and don't parse any more than necessary.
• Scanning/parsing should be unicode-aware.

Limitations

• No parsing of DDL (CREATE, DELETE, etc.). DDL often has vendor-specific quirks, and since it's atypical to have DDL on hot code paths, effort:reward ratio is not worthwhile.
• To begin with (at least), we assume parameterised queries are used, and will not attempt to remove values from queries for either aggregation or sanitisation purpose.

That is to say, in any TopN aggregations we provide, we will NOT aggregate the following queries together:

SELECT * FROM foo WHERE x=1
SELECT * FROM foo WHERE x=2

Instead, we assume that queries are parameterised like:

SELECT * FROM foo WHERE x=?

Supported database dialects

• MySQL
• PostgreSQL
• Microsoft SQL Server / T-SQL
• SQLite
• Cassandra / CQL

Others I don't have test cases for yet, but we will probably also need to cover:

• Oracle / PL-SQL
• DB2

Any others?

[axw]

A bit of a spanner in the works regarding this: "... and will not attempt to remove values from queries for either aggregation or sanitisation purpose."

@roncohen has previously found that Django won't parameterise OFFSET/LIMIT. I had forgotten about those. Some databases permit placeholders for them, and some don't. More importantly though, ORM typically just do a simple sprintf for them.

A couple of options:

1. we specifically look for LIMIT and OFFSET tokens, and replace the proceeding numbers with some placeholder, e.g. SELECT * FROM foo LIMIT 123 => SELECT * FROM foo LIMIT <number>
2. we replace all literals with placeholders, e.g. SELECT * FROM foo WHERE bar='baz' LIMIT 123 => SELECT * FROM foo WHERE bar=<string> LIMIT <number>.

I'm partial to option 2 since it's simple, and guards us from potential issues similar to the one outlines above, but perhaps it would be hiding useful low-cardinality information?

[roncohen]

The following is a bag of mixed input:

I generally think the assumption that almost all queries will be parameterized is correct, except for the LIMIT/OFFSET case. If that's true, the results of 1 and 2 are very similar, if i understood you correctly. (1) will contain a mix of "our" placeholders for LIMIT/OFFSET and placeholders that the user inserted, while (2) will be all "our" placeholders.

I think that the work involved in (1) and (2) is going to be almost the same. As far as i can tell, parsing the SQL to find OFFSET/LIMIT requires us to know the difference between literal values and language constructs and so that makes (2) interesting, because we might as well remove any remaining literals while we are at it.

From a security perspective, it could be useful to know which literals we've removed and which were already placeholders.

If we leave parameter-indicators/placeholders alone completely, we would end up with something like:

SELECT * FROM foo WHERE bar=? LIMIT <number>

Here, we can know the difference because our placeholders look different (I've seen both %s and ? being used as placeholders across different frameworks/drivers), but I wonder if that clear enough.

?/%s: placeholder already in place
<number>: placeholder we added instead of littoral

We could also consider adding a separate additional field to the data we send up to make it very clear. E.g. nonplaceholder_values: true

As a side note, if we're going to parse the SQL, it could be useful to think about sending up a parsed representation of the SQL in addition to the actual SQL. There are a number of things you might want to be able to search for.

[axw]

(1) will contain a mix of "our" placeholders for LIMIT/OFFSET and placeholders that the user inserted, while (2) will be all "our" placeholders.

I think I was unclear. In both options the user-specified placeholders would be left alone. I just meant that we could specifically look for LIMIT/OFFSET and replace the subsequent literal(s), or we could replace all literals.

i.e. given SELECT * FROM foo WHERE bar=? AND baz=42 WHERE LIMIT 3

(1) would replace it with SELECT * FROM foo WHERE bar=? AND baz=42 LIMIT <number>
(2) would replace it with SELECT * FROM foo WHERE bar=? AND baz=<number> LIMIT <number>

I prefer the latter, and it sounds like you're on board with that too.

[roncohen]

ah, gotcha. Yes, agreed 👍

[axw]

Here's a doc capturing my thoughts, and with an outline of the rules for formulating aggregation keys and span names: https://docs.google.com/document/d/1sblkAP1NHqk4MtloUta7tXjDuI_l64sT2ZQ_UFHuytA

I've just extracted the test cases from my Go unit tests into JSON files.

• Scanner: https://github.com/elastic/apm-agent-go/blob/master/internal/sqlscanner/testdata/tests.json
• Parser for signature/span name formatting: https://github.com/elastic/apm-agent-go/blob/master/internal/sqlutil/testdata/tests.json

Note that I have not yet implemented the code for formulating aggregation keys for Top N. Once you have implemented the scanner, this should be straight forward: scan through the input, replacing literals with placeholders.

[roncohen]

Looks good @axw!

[alvarolobato]

@elastic/apm-agent-devs this issue can be considered done, if you haven't had a look at it yet, please do so by the end of the week.
We will then close this issue and open an implementation one.