Closed Bug 1357431 Opened 3 years ago Closed 3 years ago

Signature generator for client-side crash stacks needs review

Categories

(Toolkit :: Telemetry, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: ddurst, Assigned: chutten)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fce-active-legacy])

The FCE team has created a prototype for batch process signatur....izing client-side submitted crash stacks (HT agashlin). Before handing to the Data Pipeline team to Scala....ize it, we should get some review.

https://gist.github.com/hcs64/229af74aabe0ef49a4a94c6c87299a6e

Adam notes that on one atmo worker, it gets through ~28.5K pings with stacks in ~35 minutes.

CCing the whole world.
Tempting to raise ValueError for the "missing payload or missing stackTraces" to get in on that sweet error goodness, even if it is just because the crash comes from a somewhat-old version. If only so we can get a count?

Shouldn't you be using CONSTANT_STYLE for constants like maximum_frames_to_consider? (not sure what PEP8 recommends)

Those TODOs suck. Are there bugs for those?

Why is this commented-out? The ping ought to have information on if it is a hang.
# Add a special marker for hang crash reports.
    #if hang_type:
    #    new_signature_list.insert(0, self.hang_prefixes[hang_type])

Looks like this was left in unintentionally
 #with open('/tmp/sig_samples/ping_%s.sig' % ping['id'], 'w') as f:
        #    f.write(signature)

FWIW you don't need to import operator for the "take a countByKey and turn it into a sorted list of tuples" pattern. You can instead use key=lambda x: x[1]

It might be nice to get an error rate. I realize 15 out of 25453 pings is a minuscule rate and is one we can collect in full, but some days might not be as happy :S

---

tl;dr - r+, this is great work. I can't wait until it's put into production. 35min for a full day's crashes on beta? Sounds nearly fast enough to be placed inline with CrashSummary. And this was beta 53, so this even includes content crash pings (bug 1293656)
Is this sufficient in review here?
Or is anything else needed?
Priority: -- → P2
Flags: needinfo?(ddurst)
I think it's good. Will verify later today.
Flags: needinfo?(ddurst)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fce-active] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.