sql: add SQLInstanceIDs to telemetry SampledQuery event #106753
Conversation
j82w
requested review from
abarganier and
DrewKimball
and removed request for
a team
|
This change is |
| for sqlId := range queryLevelStats.SqlInstanceIds { | ||
| sqlInstanceIDs = append(sqlInstanceIDs, sqlId.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
May I recommend you make the event type contain an array of integers (instead of an array of strings), then include the queryLevelStats.SqlInstanceIds slice directly into the event without conversion to string.
|
Previously, knz (Raphael 'kena' Poss) wrote…
I went with a string for the following reasons. Let me know if you think I should still change it to an int?
|
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @DrewKimball, and @j82w)
pkg/sql/exec_log.go line 332 at r1 (raw file):
The type is a set using a map
map[base.SQLInstanceID]struct{}so it would still need to be converted to an array.
Ah I had missed that. But converting a list of integers (which SQLInstanceID is under the hood - it's just an int32!) is cheaper than converting them to a list of strings.
queryLevelStats.SqlInstanceIdsshouldn't be included directly because the type isbase.SQLInstanceIDtype. We don't want external users taking a dependency on that type.
Agreed. The protobuf / event def would be defined to contain a list of integers, not SQLInstanceIDs.
The string function has custom logic to represent
0as?. It's obvious that 0 is an invalid number.
Why did you find it important to underline that? What does it buy you?
(I think it would be counter-productive to take the value 0 and put it in a structured log event.
If ever encounter 0 in production or a test, it is indicative of a bug! I'd be ok if we would just error out loudly if we ever find 0 in there.)
The current auto generated logic only supports repeated strings for some reason. Not sure if there is a reason for this.
There is already support for arrays of different types (specifically, string, uint32, LevelStats and protobuf). They are listed in log/eventpb/eventpbgen/gen.go and you can easily add a new one.
(Although for SQL instance IDs, you could just make-do with arrays of uint32 for now and rely on the existing code.)
|
Most users and devs looking at logs won't know that 0 is a invalid issue that points to there being a bug. When possible I prefer to make it specific and avoid tribal knowledge.
I disagree because putting a 0 is the only way to find possible bugs. If we don't log the event or don't include the sql instance id it just makes it ambiguous for everyone. This is writing a log message so we don't want it fail the query or do anything to impact the workload. It's better to just log the bad number and to find it then to possibly cause an issue stopping a customer workload. |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3.
Reviewable status:
-- commits line 5 at r3:
nit: (here and below) i've noticed you often have heading spaces at the start of lines. I don't know where this comes from? but it stands out compared to the rest of the git log.
pkg/sql/exec_log.go line 332 at r1 (raw file):
Previously, j82w (Jake) wrote…
Done.
Why did you find it important to underline that? What does it buy you?
Most users and devs looking at logs won't know that 0 is a invalid issue that points to there being a bug. When possible I prefer to make it specific and avoid tribal knowledge.
(I think it would be counter-productive to take the value 0 and put it in a structured log event.
If ever encounter 0 in production or a test, it is indicative of a bug! I'd be ok if we would just error out loudly if we ever find 0 in there.)I disagree because putting a 0 is the only way to find possible bugs. If we don't log the event or don't include the sql instance id it just makes it ambiguous for everyone. This is writing a log message so we don't want it fail the query or do anything to impact the workload. It's better to just log the bad number and to find it then to possibly cause an issue stopping a customer workload.
👍
Previously, knz (Raphael 'kena' Poss) wrote…
I'll fix it. I didn't notice it. I have vs code as my git editor and it seems like it adds it automatically. Not sure why. |
This adds the SQLInstanceIDs which shows which SQLInstanceIDs the statement was executed on to the SampledQuery event. This aligns the data in statement statistics and the SampledQuery event. Closes: #106719 Release note (bug fix): Added the missing SQLInstanceIDs used to execute the statement to the telemetry SampledQuery event.
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 6e12082 to blathers/backport-release-23.1-106753: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This adds the SQLInstanceIDs which shows which SQLInstanceIDs the
statement was executed on to the SampledQuery event. This aligns the
data in statement statistics and the SampledQuery event.
Closes: #106719
Release note (bug fix): Added the missing SQLInstanceIDs used to
execute the statement to the telemetry SampledQuery event.