[PATCH 0/3] Avoid false positives when deduplicating very short core backtraces

[Martin Milata]

According to [1], whenever a crash happens on a machine and an unrelated crash
happened previously and both of them had core backtrace with only one frame,
they would get classified as duplicates, regardless whether the frames are
different or not.

The deduplication routine measures levenshtein distance between the two
backtraces and whenever the result is less or equal than two, the backtraces
are marked as duplicates. Levenshtein distance denotes the number of operations
required to get one sequence from another, so it is always <= 1 for backtraces
of length 1 and <= 2 for backtraces of length 2.

One solution is to divide the distance by the length of the backtrace and then
decide based on this relative number of frames. AFAIK we use this method for
server-side clustering, and it is also the default deduplication method in
satyr (backtraces with less than 30% different frames are marked duplicates).

I decided to implement another fix - check whether at least one of the
backtraces is shorter than three frames, and if it is, perform a full
comparison of the backtraces. If the backtraces are longer, use the usual
method. The reason is not breaking the ABRT in RHEL6 too much (no big
adjustment to the algorithm), while the first solution will be used with the
switch to satyr anyway (I expect that some debugging and adjustments will be
necessary).

Let me know what you think and I'll redo the patch. Sorry for so much text.

[1] https://github.com/abrt/btparser/issues/6

Martin Milata (3):
  Only problems of same type can be duplicates
  satyr-compat: treat bt distance as a float
  Short BT deduplication false positives workaround

 src/daemon/abrt-handle-event.c | 39 ++++++++++++++++++++++++++++++++++-----
 src/include/satyr-compat.h     | 10 ++++++++--
 2 files changed, 42 insertions(+), 7 deletions(-)

-- 
1.7.11.7

[PATCH 1/3] Only problems of same type can be duplicates

[Martin Milata]

The local duplicate detection routine was augmented to never consider
two problems from different analyzer to be duplicates.

Signed-off-by: Martin Milata <mmilata(a)redhat.com&gt;
---
 src/daemon/abrt-handle-event.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/daemon/abrt-handle-event.c b/src/daemon/abrt-handle-event.c
index e11d192..11d554f 100644
--- a/src/daemon/abrt-handle-event.c
+++ b/src/daemon/abrt-handle-event.c
@@ -197,6 +197,10 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
     dup_uuid_init(dd);
     dup_corebt_init(dd);
 
+    char *analyzer = dd_load_text(dd, FILENAME_ANALYZER);
+    /* dump_dir_name can be relative */
+    dump_dir_name = realpath(dump_dir_name, NULL);
+
     dd_close(dd);
 
     DIR *dir = opendir(g_settings_dump_location);
@@ -215,6 +219,7 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
             continue; /* skip anything named "<dirname>.new" */
 
         char *dump_dir_name2 = concat_path_file(g_settings_dump_location,
dent->d_name);
+        char *dd_uid = NULL, *dd_analyzer = NULL;
 
         if (strcmp(dump_dir_name, dump_dir_name2) == 0)
             goto next; /* we are never a dup of ourself */
@@ -224,13 +229,21 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
             goto next;
 
         /* crashes of different users are not considered duplicates */
-        char *dd_uid = dd_load_text_ext(dd, FILENAME_UID, DD_FAIL_QUIETLY_ENOENT);
+        dd_uid = dd_load_text_ext(dd, FILENAME_UID, DD_FAIL_QUIETLY_ENOENT);
         if (strcmp(uid, dd_uid))
         {
             dd_close(dd);
             goto next;
         }
 
+        /* different crash types are not duplicates */
+        dd_analyzer = dd_load_text_ext(dd, FILENAME_ANALYZER, DD_FAIL_QUIETLY_ENOENT);
+        if (strcmp(analyzer, dd_analyzer))
+        {
+            dd_close(dd);
+            goto next;
+        }
+
         if (dup_uuid_compare(dd)
          || dup_corebt_compare(dd)
         ) {
@@ -243,10 +256,13 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
 
 next:
         free(dump_dir_name2);
+        free(dd_uid);
+        free(dd_analyzer);
     }
     closedir(dir);
 
 end:
+    free(analyzer);
     return retval;
 }
 
-- 
1.7.11.7

[PATCH 2/3] satyr-compat: treat bt distance as a float

[Martin Milata]

It is float in satyr, while btparser returns int.

Signed-off-by: Martin Milata <mmilata(a)redhat.com&gt;
---
 src/daemon/abrt-handle-event.c | 6 +++---
 src/include/satyr-compat.h     | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/daemon/abrt-handle-event.c b/src/daemon/abrt-handle-event.c
index 11d554f..92e2c8f 100644
--- a/src/daemon/abrt-handle-event.c
+++ b/src/daemon/abrt-handle-event.c
@@ -49,8 +49,8 @@ static int core_backtrace_is_duplicate(struct sr_core_stacktrace *bt1,
         goto end;
     }
 
-    int distance = sr_distance_core(SR_DISTANCE_DAMERAU_LEVENSHTEIN,
-                                    thread1, thread2);
+    float distance = sr_distance_core(SR_DISTANCE_DAMERAU_LEVENSHTEIN,
+                                      thread1, thread2);
 
     if (distance == -1)
     {
@@ -58,7 +58,7 @@ static int core_backtrace_is_duplicate(struct sr_core_stacktrace *bt1,
     }
     else
     {
-        VERB2 log("Distance between backtraces: %d", distance);
+        VERB2 log("Distance between backtraces: %f", distance);
         result = (distance <= BACKTRACE_DUP_THRESHOLD);
     }
 
diff --git a/src/include/satyr-compat.h b/src/include/satyr-compat.h
index 0752785..ad05dbd 100644
--- a/src/include/satyr-compat.h
+++ b/src/include/satyr-compat.h
@@ -55,12 +55,11 @@
 #include <btparser/location.h>
 #include "libabrt.h" /* xstrdup */
 
-#define BACKTRACE_DUP_THRESHOLD 2
+#define BACKTRACE_DUP_THRESHOLD 2.0
 
 /* abrt-handle-event.c */
 #define sr_core_stacktrace btp_thread
 #define sr_core_thread btp_thread
-#define sr_thread_get_frame_count btp_thread_get_frame_count
 
 enum sr_distance_type
 {
-- 
1.7.11.7

[PATCH 3/3] Short BT deduplication false positives workaround

[Martin Milata]

Whenever backtraces that are shorter or equal than two frames are
checked for duplicity, fall back to full comparison. This should prevent
the current behaviour that two one-frame backtraces are always
duplicates regardless of the frame content.

Fixes abrt/btparser#6.

Signed-off-by: Martin Milata <mmilata(a)redhat.com&gt;
---
 src/daemon/abrt-handle-event.c | 15 ++++++++++++++-
 src/include/satyr-compat.h     |  7 +++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/daemon/abrt-handle-event.c b/src/daemon/abrt-handle-event.c
index 92e2c8f..521a7e1 100644
--- a/src/daemon/abrt-handle-event.c
+++ b/src/daemon/abrt-handle-event.c
@@ -42,13 +42,26 @@ static int core_backtrace_is_duplicate(struct sr_core_stacktrace
*bt1,
     struct sr_core_thread *thread1 = sr_core_stacktrace_find_crash_thread(bt1);
     struct sr_core_thread *thread2 = sr_core_stacktrace_find_crash_thread(bt2);
 
-    if (sr_core_thread_get_frame_count(thread2) <= 0)
+    int length1 = sr_core_thread_get_frame_count(thread1);
+    int length2 = sr_core_thread_get_frame_count(thread2);
+
+    if (length2 <= 0)
     {
         VERB1 log("Core backtrace has zero frames, considering it not
duplicate");
         result = 0;
         goto end;
     }
 
+    /* This is an ugly workaround for https://github.com/abrt/btparser/issues/6 */
+#ifndef USE_SATYR
+    if (length1 <= 2 || length2 <= 2)
+    {
+        VERB1 log("Backtraces too short, falling back on full comparison");
+        result = (sr_core_thread_cmp(thread1, thread2) == 0);
+        goto end;
+    }
+#endif
+
     float distance = sr_distance_core(SR_DISTANCE_DAMERAU_LEVENSHTEIN,
                                       thread1, thread2);
 
diff --git a/src/include/satyr-compat.h b/src/include/satyr-compat.h
index ad05dbd..23f6bb7 100644
--- a/src/include/satyr-compat.h
+++ b/src/include/satyr-compat.h
@@ -96,6 +96,13 @@ sr_core_thread_get_frame_count(struct sr_core_thread *thread)
     return btp_thread_get_frame_count(thread);
 }
 
+int
+sr_core_thread_cmp(struct sr_core_thread *thread1,
+                   struct sr_core_thread *thread2)
+{
+    return btp_thread_cmp(thread1, thread2);
+}
+
 float
 sr_distance_core(enum sr_distance_type distance_type,
                  struct sr_core_thread *thread1,
-- 
1.7.11.7

[ABRT PATCH 0/3]

[Martin Milata]

These patches are for ABRT.

[ABRT PATCH 0/3]

[Jakub Filak]

Good job! Pushed. Thanks!

----- Original Message -----
From: "Martin Milata" <mmilata(a)redhat.com&gt;
To: crash-catcher(a)lists.fedorahosted.org
Sent: Friday, May 10, 2013 4:11:57 PM
Subject: Re: [ABRT PATCH 0/3]

These patches are for ABRT.