Why query in OpenTelemetry span is limited by 5k characters?

[iorlas]

Hey! We are evaluating OpenTelemetry right now. Feels a bit wacky, with many moving pieces, but now it feels maturish at least.

We are interested in running all the deps with OpenTelemetry so it was really nice to discover go-pg to have such a module ready-to-use.

We are on the early stages, we kind of assume there will be some issues. The first one we have discovered is all queries are limited by 5k characters:

pg/extra/pgotel/pgotel.go

Line 77 in b3797bd

const queryLimit = 5000

Although it is not a dealbreaker, it would be nice for us to understand the origin of such a limitation. The major issue with such truncation is the inability to observe the actual arguments passed. Is it the protocol, common sense, or anything else?

Since it is the real issue, we see multiple solutions:

• If len(query) > 5000, strip down all the columns in SELECT statement, all the data in SET (UPDATE/INSERT) statement. Simply put, if we need to lose any data, let's lose only the less significant information first. We've been using such an approach with other tracing software.
• Expand the limit to 10k. Might be too costly to store, still might not be enough.
• Send arguments separately. It will help to get more context, although which arguments are which... it might get confusing. Keep in mind the fact arguments are being used for the INSERT/UPDATE statements as well, so these should be exempt from the logic
• Send arguments AND the "WHERE" expression separately. Although it will resolve 60% of cases, it will feel too wacky

I feel like we will go with the first option internally, but I personally interested in resolving this issue globally.
Any thoughts?

Also, for some reason "start discussion" on the issues open page leads to the redis discussions page. Dunno what is wrong

[vmihailenco]

What happens is:

1. INSERT has all data stripped by default using UnformattedQuery -

   pg/extra/pgotel/pgotel.go

   Lines 50 to 62 in b3797bd

   	if operation == orm.InsertOp {
   	b, err := evt.UnformattedQuery()
   	if err != nil {
   	return err
   	}
   	query = string(b)
   	} else {
   	b, err := evt.FormattedQuery()
   	if err != nil {
   	return err
   	}
   	query = string(b)
   	}

2. All the rest is truncated to 5000 on assumption that it is enough to cover 99% queries
3. In Uptrace the query is parsed and transformed according to some limits

I don't see much sense in doing 3) in go-pg because

• parsing SQL is a lot of work and is kinda slow
• tracing systems (like Jaeger/Zipkin) have to parse queries anyway because it is unrealistic/impossible to expect that every user is able to parse/prepare their queries

Send arguments separately.

It is now always possible (some queries already come pre-formatted) and even with simple Where("id = ?", id) it is not clear how to do it.

Any thoughts?

You can copy-paste the instrumentation code and edit as you wish. E.g. always using UnformattingQuery seems to achieve what you want (strip all args). But you will lose to ability to see the "real" query with all args and run it. Which is more important to me.

Also, for some reason "start discussion" on the issues open page leads to the redis discussions page. Dunno what is wrong

WIll fix - thanks.

[iorlas]

You can copy-paste the instrumentation code and edit as you wish. E.g. always using UnformattingQuery seems to achieve what you want (strip all args). But you will lose to ability to see the "real" query with all args and run it. Which is more important to me.

The key issue is the fact some queries cannot be expressed at all bc of the length. We see the query which consists of the "SELECT table1.col1, table1.col2..." only, leaving the JOINs and WHERE conditions out. I agree that an unmodified query is what should be there, but it conveys zero information in such cases. The case I mentioned is not the worst one.

Just a couple of more examples of issues when the query is truncated, for clarity:

• Predicates could be calculated dynamically, so the WHERE conditions won't be obvious even when looking into the code, so we are losing some info here.
• Query might be complex enough, prepping some CTE and using it later on, making some nested queries. What is certain here, we will lose some key data, which could make the rest of the data useless, basically.

I agree with you, the best way to handle it - adjust it on a case-by-case basis. However, if we could generalize it - it would benefit everybody. The question becomes clear: which behavior will satisfy 80% of cases.

So, just thinking out loud, here are additional ideas on improvements:

• What if there would be an option to shrink down the table aliases? What I have stumbled upon, is the huge SELECT queries, where MOST of the query was spent for the table aliases. I'm aware of the way to assign my own table alias on each table, but it is not what I'm talking about. I think, if table aliases were generated in a form of "a, b, aa, bb" (or something like it), it would save a huge amount of storage and traffic. And if you need to dig what the a or bb is, you will need to dig the query anyway.
• Introduce an options pattern for the Tracing hook, which will allow changing the behavior (truncation and the way the truncation is done by turning the mode)
• If query length > limit, apply some simple replacement, like this (actually taken from one of the real projects): regexp.MustCompile("SELECT (.*) FROM").ReplaceAll(query, []byte("SELECT <COLUMNS> FROM")). Obviously, regex puts a toll on the performance, and regex are not made to analyze and modify SQL (like HTML, hah), but similar look, we can have some options here, like substrings or query pre-generation in the query builder. Regardless of the approach, if the column names could be hidden, I think 99% of queries will fit the limit, so the most valuable info will be saved.

What are your thoughts?

[vmihailenco]

if table aliases were generated in a form of "a, b, aa, bb"

It makes queries much less readable for everyone.

Introduce an options pattern for the Tracing hook

Sounds like a sensible idea.

regexp.MustCompile("SELECT (.*) FROM").ReplaceAll(query, []byte("SELECT FROM"))

It may break queries that have multiple FROM clauses, but it may work better than simple truncation.

It is also possible to change query builder to omit columns altogether for unformatted queries. E.g. for insert we already do something like that https://github.com/go-pg/pg/blob/v10/orm/insert.go#L275-L277

[iorlas]

It makes queries much less readable for everyone.

Indeed, but it might be more suitable for the production environment, where observability and resources are critical. Anyway, even if the "a,b,c" notation will be applied, no data will be lost, which is, again, much better that losing all the "WHERE" condition information. I'm not a fan of such an approach either, but hey, it kind of makes sense to me.

Sounds like a sensible idea.

Will try to decompose some of our internal logic into options then. Hope it will help us.

It is also possible to change query builder to omit columns altogether for unformatted queries

This is what I was thinking as well, it would be the best way of resolving such an issue: the resulting query will be compiled by the query builder, which is SQL-aware and can ditch unnecessary parts w/o too much hassle, but doing it in the correct way.

Also, we have found out the otel extra module sends out the source reference information, but it tries to find an immediate caller of go-pg module. We are using zap.AddCallerSkip to control which function info should be traced. Will try to propose the PR with such an option as well.

[vmihailenco]

👍

In #1856 I've tried to improve template formatter to omit columns when there are > 10 of them.

We are using zap.AddCallerSkip

I am not sure this will work since that hook is called from multiple places. But it is worth a try.