Closed Bug 896737 Opened 6 years ago Closed 6 years ago

Limit the length of Slow SQL statements reported to Telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: vladan, Assigned: kamilm)

References

Details

(Whiteboard: [mentor=vladan][lang=c++])

Attachments

(3 files, 2 obsolete files)

Some FormHistory and Places statements can contain thousands of arguments, causing the Telemetry ping to become very large. One slowSQL section I looked at was 1.3 MBs!

We should limit the length of reported slowSQL statements to 100 chars each, and ~5KB total.
Blocks: 896744
do you have an example of these statements?
thousands of arguments doesn't look sane, we should probably also fix the backend.
(In reply to Marco Bonardo [:mak] from comment #1)
> do you have an example of these statements?
> thousands of arguments doesn't look sane, we should probably also fix the
> backend.

I attached a sample from one of the large Telemetry pings
yeah, that's horrible. it's ok to do the limitation here, but I'd also appreciate a bug to investigate alternatives to those very long lists of ids... We can probably do better.
Another example from form history
(In reply to Marco Bonardo [:mak] from comment #3)
> [...] I'd also appreciate a bug to investigate alternatives to those 
> very long lists of ids... We can probably do better.

I filed bug 897074 and bug 897081
Whiteboard: [mentor=vladan][lang=c++][good first bug] → [mentor=vladan][lang=c++]
I'd like to get involved in Firefox development. Vladan, is one of the bugs you reported above simple enough to get me started? :)
(In reply to Kamil Muszyński (:kamilm) from comment #6)
> I'd like to get involved in Firefox development. Vladan, is one of the bugs
> you reported above simple enough to get me started? :)

Hi Kamil. This bug is pretty straight-forward but it's probably not the best for a first bug. If you're feeling a little bit brave, I can still walk you through it :) You can email me at vdjeric@mozilla.com for the details
Attached patch Proposed patch - first version (obsolete) — Splinter Review
Changes only in Telemetry.cpp file
Attachment #793583 - Flags: review?(vdjeric)
Comment on attachment 793583 [details] [diff] [review]
Proposed patch - first version

This looks good :) Just a few small suggestions

>+const uint32_t kMaxSlowStatementLength = 50;

Let's make the max statement length 1000 for now actually. I took a look at my about:telemetry and there were several statements near that length. Assuming we limit Telemetry to max 50 SQL statements, that would still be only ~50KB of memory.

>   nsAutoCString fullSQL(sql);
>-  fullSQL.AppendPrintf(" /* %s */", dbName.BeginReading());
>-
>+  

You don't need to keep the fullSQL variable at the top anymore. You can just pass sql directly to sanitizeSQL() and declare fullSQL closer to where it is used (the last call to StoreSlowSQL).

>+  nsAutoCString dbNameComment;
>+  dbNameComment.AppendPrintf(" /* %s */", dbName.BeginReading());
>+  
>   bool isFirefoxDB = sTelemetry->mTrackedDBs.Contains(dbName);
>   if (isFirefoxDB) {
>     nsAutoCString sanitizedSQL(SanitizeSQL(fullSQL));
>+    if (sanitizedSQL.Length() + dbNameComment.Length() > kMaxSlowStatementLength ){
>+	  sanitizedSQL.SetLength(kMaxSlowStatementLength - dbNameComment.Length() - 3);	// minus 3 to add space for '...'

It's unlikely to happen, but if someone changed kMaxSlowStatementLength to some smaller value (like 20) and there was a database with a long filename (e.g. 25 chars), the calculated length passed to SetLength() would be negative. And since SetLength takes an unsigned int, the negative number might get interpreted as a very large positive number. 
I suggest we just apply the maximum length to the SQL portion of the string, and disregard the length of the DB filename portion.
Attachment #793583 - Flags: review?(vdjeric)
Assignee: nobody → muszynski.kamil
Hi Vladan,
I've attached second patch for Telemetry.cpp for a review. Trimming is now only applied to the SQL statement, after that the ("..." + DB name) string is added and stored in Telemetry. I've also made a comment for kMaxSlowStatementLength constant, as it now may be a little misleading, since the actual length of a string that is stored may be greater than kMaxSlowStatementLength:)
Attachment #793583 - Attachment is obsolete: true
Attachment #795180 - Flags: review?(vdjeric)
Comment on attachment 795180 [details] [diff] [review]
Proposed changes in Telemetry.cpp - after the first review

A few small style comments:
- There are trailing spaces at the end of several lines, remove them please
- Indent using 2 spaces in this file, don't use tabs
- spacing in an if-statement is as follows: "if (some_condition) {"
- Start comment lines with only two slashes: // an example comment

For future reference, the coding style can be found here: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
Attachment #795180 - Flags: review?(vdjeric)
Attachment #795180 - Attachment is obsolete: true
Attachment #797438 - Flags: review?(vdjeric)
Attachment #797438 - Flags: review?(vdjeric) → review+
Congrats on your first patch, Kamil :)

I was also planning to limit the number of unique slow SQL statements reported per session in this bug, but I've decided against it.. I don't think the limit will get used often (if at all) and even if it did somehow, we still wouldn't want to lose any of the data. I'm marking the bug resolved.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Don't resolve bugs until they hit m-c. We have merge scripts that handle the job and ensure that details don't get missed in the process.
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.