]> arthur.barton.de Git - netdata.git/commitdiff
fixed a bug in the interpolation algorithm that was showing unrealistic values under...
authorCosta Tsaousis (ktsaou) <costa@tsaousis.gr>
Wed, 21 Sep 2016 23:17:14 +0000 (02:17 +0300)
committerCosta Tsaousis (ktsaou) <costa@tsaousis.gr>
Wed, 21 Sep 2016 23:17:14 +0000 (02:17 +0300)
src/rrd.c
src/unit_test.c

index acb98021bebf295497819325584d3bd5405a11a6..e72f495d157dee890c7ce8668945a4c434a6318d 100644 (file)
--- a/src/rrd.c
+++ b/src/rrd.c
@@ -988,7 +988,7 @@ unsigned long long rrdset_done(RRDSET *st)
 
     RRDDIM *rd, *last;
     int oldstate, store_this_entry = 1, first_entry = 0;
-    unsigned long long last_ut, now_ut, next_ut, stored_entries = 0;
+    unsigned long long last_stored_ut, now_collect_ut, last_collect_ut, next_store_ut, stored_entries = 0;
 
     if(unlikely(pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate) != 0))
         error("Cannot set pthread cancel state to DISABLE.");
@@ -1014,6 +1014,7 @@ unsigned long long rrdset_done(RRDSET *st)
         // it is the first entry
         // set the last_collected_time to now
         gettimeofday(&st->last_collected_time, NULL);
+        last_collect_ut = st->last_collected_time.tv_sec * 1000000ULL + st->last_collected_time.tv_usec - (st->update_every * 1000000);
 
         // the first entry should not be stored
         store_this_entry = 0;
@@ -1024,7 +1025,8 @@ unsigned long long rrdset_done(RRDSET *st)
     else {
         // it is not the first entry
         // calculate the proper last_collected_time, using usec_since_last_update
-        unsigned long long ut = st->last_collected_time.tv_sec * 1000000ULL + st->last_collected_time.tv_usec + st->usec_since_last_update;
+        last_collect_ut = st->last_collected_time.tv_sec * 1000000ULL + st->last_collected_time.tv_usec;
+        unsigned long long ut = last_collect_ut + st->usec_since_last_update;
         st->last_collected_time.tv_sec = (time_t) (ut / 1000000ULL);
         st->last_collected_time.tv_usec = (suseconds_t) (ut % 1000000ULL);
     }
@@ -1064,21 +1066,18 @@ unsigned long long rrdset_done(RRDSET *st)
     }
 
     // these are the 3 variables that will help us in interpolation
-    // last_ut = the last time we added a value to the storage
-    //  now_ut = the time the current value is taken at
-    // next_ut = the time of the next interpolation point
-    last_ut = st->last_updated.tv_sec * 1000000ULL + st->last_updated.tv_usec;
-    now_ut  = st->last_collected_time.tv_sec * 1000000ULL + st->last_collected_time.tv_usec;
-    next_ut = (st->last_updated.tv_sec + st->update_every) * 1000000ULL;
-
-    if(unlikely(!first_entry && now_ut < next_ut)) {
-        if(unlikely(st->debug)) debug(D_RRD_STATS, "%s: THIS IS IN THE SAME INTERPOLATION POINT", st->name);
-    }
+    // last_stored_ut = the last time we added a value to the storage
+    // now_collect_ut = the time the current value has been collected
+    // next_store_ut  = the time of the next interpolation point
+    last_stored_ut = st->last_updated.tv_sec * 1000000ULL + st->last_updated.tv_usec;
+    now_collect_ut = st->last_collected_time.tv_sec * 1000000ULL + st->last_collected_time.tv_usec;
+    next_store_ut  = (st->last_updated.tv_sec + st->update_every) * 1000000ULL;
 
     if(unlikely(st->debug)) {
-        debug(D_RRD_STATS, "%s: last ut = %0.3Lf (last updated time)", st->name, (long double)last_ut/1000000.0);
-        debug(D_RRD_STATS, "%s: now  ut = %0.3Lf (current update time)", st->name, (long double)now_ut/1000000.0);
-        debug(D_RRD_STATS, "%s: next ut = %0.3Lf (next interpolation point)", st->name, (long double)next_ut/1000000.0);
+        debug(D_RRD_STATS, "%s: last_collect_ut = %0.3Lf (last collection time)", st->name, (long double)last_collect_ut/1000000.0);
+        debug(D_RRD_STATS, "%s: now_collect_ut  = %0.3Lf (current collection time)", st->name, (long double)now_collect_ut/1000000.0);
+        debug(D_RRD_STATS, "%s: last_stored_ut  = %0.3Lf (last updated time)", st->name, (long double)last_stored_ut/1000000.0);
+        debug(D_RRD_STATS, "%s: next_store_ut   = %0.3Lf (next interpolation point)", st->name, (long double)next_store_ut/1000000.0);
     }
 
     if(unlikely(!st->counter_done)) {
@@ -1090,7 +1089,7 @@ unsigned long long rrdset_done(RRDSET *st)
     // calculate totals and count the dimensions
     int dimensions;
     st->collected_total = 0;
-    for( rd = st->dimensions, dimensions = 0 ; likely(rd) ; rd = rd->next, dimensions++ )
+    for( rd = st->dimensions, dimensions = 0 ; rd ; rd = rd->next, dimensions++ )
         if(likely(rd->updated)) st->collected_total += rd->collected_value;
 
     uint32_t storage_flags = SN_EXISTS;
@@ -1098,7 +1097,7 @@ unsigned long long rrdset_done(RRDSET *st)
     // process all dimensions to calculate their values
     // based on the collected figures only
     // at this stage we do not interpolate anything
-    for( rd = st->dimensions ; likely(rd) ; rd = rd->next ) {
+    for( rd = st->dimensions ; rd ; rd = rd->next ) {
 
         if(unlikely(!rd->updated)) {
             rd->calculated_value = 0;
@@ -1177,7 +1176,7 @@ unsigned long long rrdset_done(RRDSET *st)
                     rd->last_collected_value = rd->collected_value;
                 }
 
-                rd->calculated_value =
+                rd->calculated_value +=
                       (calculated_number)(rd->collected_value - rd->last_collected_value)
                     * (calculated_number)rd->multiplier
                     / (calculated_number)rd->divisor;
@@ -1267,21 +1266,26 @@ unsigned long long rrdset_done(RRDSET *st)
     // at this point we have all the calculated values ready
     // it is now time to interpolate values on a second boundary
 
-    unsigned long long first_ut = last_ut;
-    long long iterations = (now_ut - last_ut) / (st->update_every * 1000000ULL);
-    if((now_ut % (st->update_every * 1000000ULL)) == 0) iterations++;
+    if(unlikely(now_collect_ut < next_store_ut)) {
+        // this is collected in the same interpolation point
+        if(unlikely(st->debug)) debug(D_RRD_STATS, "%s: THIS IS IN THE SAME INTERPOLATION POINT", st->name);
+    }
+
+    unsigned long long first_ut = last_stored_ut;
+    long long iterations = (now_collect_ut - last_stored_ut) / (st->update_every * 1000000ULL);
+    if((now_collect_ut % (st->update_every * 1000000ULL)) == 0) iterations++;
 
-    for( ; likely(next_ut <= now_ut) ; next_ut += st->update_every * 1000000ULL, iterations-- ) {
+    for( ; next_store_ut <= now_collect_ut ; next_store_ut += st->update_every * 1000000ULL, iterations-- ) {
 #ifdef NETDATA_INTERNAL_CHECKS
-        if(iterations < 0) { error("%s: iterations calculation wrapped! first_ut = %llu, last_ut = %llu, next_ut = %llu, now_ut = %llu", st->name, first_ut, last_ut, next_ut, now_ut); }
+        if(iterations < 0) { error("%s: iterations calculation wrapped! first_ut = %llu, last_stored_ut = %llu, next_store_ut = %llu, now_collect_ut = %llu", st->name, first_ut, last_stored_ut, next_store_ut, now_collect_ut); }
 #endif
 
         if(unlikely(st->debug)) {
-            debug(D_RRD_STATS, "%s: last ut = %0.3Lf (last updated time)", st->name, (long double)last_ut/1000000.0);
-            debug(D_RRD_STATS, "%s: next ut = %0.3Lf (next interpolation point)", st->name, (long double)next_ut/1000000.0);
+            debug(D_RRD_STATS, "%s: last_stored_ut = %0.3Lf (last updated time)", st->name, (long double)last_stored_ut/1000000.0);
+            debug(D_RRD_STATS, "%s: next_store_ut  = %0.3Lf (next interpolation point)", st->name, (long double)next_store_ut/1000000.0);
         }
 
-        st->last_updated.tv_sec = (time_t) (next_ut / 1000000ULL);
+        st->last_updated.tv_sec = (time_t) (next_store_ut / 1000000ULL);
         st->last_updated.tv_usec = 0;
 
         for( rd = st->dimensions ; likely(rd) ; rd = rd->next ) {
@@ -1291,8 +1295,8 @@ unsigned long long rrdset_done(RRDSET *st)
                 case RRDDIM_INCREMENTAL:
                     new_value = (calculated_number)
                         (      rd->calculated_value
-                            * (calculated_number)(next_ut - last_ut)
-                            / (calculated_number)(now_ut - last_ut)
+                            * (calculated_number)(next_store_ut - last_collect_ut)
+                            / (calculated_number)(now_collect_ut - last_collect_ut)
                         );
 
                     if(unlikely(st->debug))
@@ -1304,10 +1308,11 @@ unsigned long long rrdset_done(RRDSET *st)
                             , st->id, rd->name
                             , new_value
                             , rd->calculated_value
-                            , (next_ut - last_ut)
-                            , (now_ut - last_ut)
+                            , (next_store_ut - last_stored_ut)
+                            , (now_collect_ut - last_stored_ut)
                             );
 
+                    last_collect_ut = next_store_ut;
                     rd->calculated_value -= new_value;
                     new_value += rd->last_calculated_value;
                     rd->last_calculated_value = 0;
@@ -1331,8 +1336,8 @@ unsigned long long rrdset_done(RRDSET *st)
 
                         new_value = (calculated_number)
                             (   (     (rd->calculated_value - rd->last_calculated_value)
-                                    * (calculated_number)(next_ut - first_ut)
-                                    / (calculated_number)(now_ut - first_ut)
+                                    * (calculated_number)(next_store_ut - last_collect_ut)
+                                    / (calculated_number)(now_collect_ut - last_collect_ut)
                                 )
                                 +  rd->last_calculated_value
                             );
@@ -1346,22 +1351,15 @@ unsigned long long rrdset_done(RRDSET *st)
                                 , st->id, rd->name
                                 , new_value
                                 , rd->calculated_value, rd->last_calculated_value
-                                , (next_ut - first_ut)
-                                , (now_ut - first_ut), rd->last_calculated_value
+                                , (next_store_ut - first_ut)
+                                , (now_collect_ut - first_ut), rd->last_calculated_value
                                 );
-
-                        // this is wrong
-                        // it fades the value towards the target
-                        // while we know the calculated value is different
-                        // if(likely(next_ut + st->update_every * 1000000ULL > now_ut)) rd->calculated_value = new_value;
                     }
                     break;
             }
 
-            if(unlikely(!store_this_entry)) {
-                // store_this_entry = 1;
+            if(unlikely(!store_this_entry))
                 continue;
-            }
 
             if(likely(rd->updated && rd->counter > 1 && iterations < st->gap_when_lost_iterations_above)) {
                 rd->values[st->current_entry] = pack_storage_number(new_value, storage_flags );
@@ -1419,25 +1417,34 @@ unsigned long long rrdset_done(RRDSET *st)
 
         st->counter++;
         st->current_entry = ((st->current_entry + 1) >= st->entries) ? 0 : st->current_entry + 1;
-        last_ut = next_ut;
+        last_stored_ut = next_store_ut;
     }
 
-    // align next interpolation to last collection point
-    if(likely(stored_entries || !store_this_entry)) {
-        st->last_updated.tv_sec = st->last_collected_time.tv_sec;
-        st->last_updated.tv_usec = st->last_collected_time.tv_usec;
-        st->last_collected_total  = st->collected_total;
-    }
+    st->last_collected_total  = st->collected_total;
 
-    for( rd = st->dimensions; likely(rd) ; rd = rd->next ) {
+    for( rd = st->dimensions; rd ; rd = rd->next ) {
         if(unlikely(!rd->updated)) continue;
 
-        if(likely(stored_entries || !store_this_entry)) {
-            if(unlikely(st->debug)) debug(D_RRD_STATS, "%s/%s: setting last_collected_value (old: " COLLECTED_NUMBER_FORMAT ") to last_collected_value (new: " COLLECTED_NUMBER_FORMAT ")", st->id, rd->name, rd->last_collected_value, rd->collected_value);
-            rd->last_collected_value = rd->collected_value;
+        if(unlikely(st->debug)) debug(D_RRD_STATS, "%s/%s: setting last_collected_value (old: " COLLECTED_NUMBER_FORMAT ") to last_collected_value (new: " COLLECTED_NUMBER_FORMAT ")", st->id, rd->name, rd->last_collected_value, rd->collected_value);
+        rd->last_collected_value = rd->collected_value;
+
+        switch(rd->algorithm) {
+            case RRDDIM_INCREMENTAL:
+                if(unlikely(!first_entry)) {
+                    if(unlikely(st->debug)) debug(D_RRD_STATS, "%s/%s: setting last_calculated_value (old: " CALCULATED_NUMBER_FORMAT ") to last_calculated_value (new: " CALCULATED_NUMBER_FORMAT ")", st->id, rd->name, rd->last_calculated_value + rd->calculated_value, rd->calculated_value);
+                    rd->last_calculated_value += rd->calculated_value;
+                }
+                else {
+                    if(unlikely(st->debug)) debug(D_RRD_STATS, "%s: THIS IS THE FIRST POINT", st->name);
+                }
+                break;
 
-            if(unlikely(st->debug)) debug(D_RRD_STATS, "%s/%s: setting last_calculated_value (old: " CALCULATED_NUMBER_FORMAT ") to last_calculated_value (new: " CALCULATED_NUMBER_FORMAT ")", st->id, rd->name, rd->last_calculated_value, rd->calculated_value);
-            rd->last_calculated_value = rd->calculated_value;
+            case RRDDIM_ABSOLUTE:
+            case RRDDIM_PCENT_OVER_ROW_TOTAL:
+            case RRDDIM_PCENT_OVER_DIFF_TOTAL:
+                if(unlikely(st->debug)) debug(D_RRD_STATS, "%s/%s: setting last_calculated_value (old: " CALCULATED_NUMBER_FORMAT ") to last_calculated_value (new: " CALCULATED_NUMBER_FORMAT ")", st->id, rd->name, rd->last_calculated_value, rd->calculated_value);
+                rd->last_calculated_value = rd->calculated_value;
+                break;
         }
 
         rd->calculated_value = 0;
index a77fdb1349b60a98228b7fc64c3da54f7915c2f8..843fc877c0ca064fd7a3780501121e4dd9aad849 100644 (file)
@@ -736,6 +736,72 @@ struct test test13 = {
         NULL                // results2
 };
 
+// --------------------------------------------------------------------------------------------------------------------
+// test14
+
+struct feed_values test14_feed[] = {
+        {        0, 0x015397dc42151c41ULL },
+        { 13573000, 0x015397e612e3ff5dULL },
+        { 29969000, 0x015397f905ecdaa8ULL },
+        { 29958000, 0x0153980c2a6cb5e4ULL },
+        { 30054000, 0x0153981f4032fb83ULL },
+        { 34952000, 0x015398355efadaccULL },
+        { 25046000, 0x01539845ba4b09f8ULL },
+        { 29947000, 0x0153985948bf381dULL },
+        { 30054000, 0x0153986c5b9c27e2ULL },
+        { 29942000, 0x0153987f888982d0ULL },
+};
+
+calculated_number test14_results[] = {
+        23.1383300, 21.8515600, 21.8804600, 21.7788000, 22.0112200, 22.4386100, 22.0906100, 21.9150800
+};
+
+struct test test14 = {
+        "test14",            // name
+        "issue #981 with real data",
+        30,                 // update_every
+        8,                  // multiplier
+        1000000000,         // divisor
+        RRDDIM_INCREMENTAL, // algorithm
+        10,                 // feed entries
+        8,                  // result entries
+        test14_feed,        // feed
+        test14_results,     // results
+        NULL,               // feed2
+        NULL                // results2
+};
+struct feed_values test14b_feed[] = {
+        {        0, 0 },
+        { 13573000, 13573000 },
+        { 29969000, 13573000 + 29969000 },
+        { 29958000, 13573000 + 29969000 + 29958000 },
+        { 30054000, 13573000 + 29969000 + 29958000 + 30054000 },
+        { 34952000, 13573000 + 29969000 + 29958000 + 30054000 + 34952000 },
+        { 25046000, 13573000 + 29969000 + 29958000 + 30054000 + 34952000 + 25046000 },
+        { 29947000, 13573000 + 29969000 + 29958000 + 30054000 + 34952000 + 25046000 + 29947000 },
+        { 30054000, 13573000 + 29969000 + 29958000 + 30054000 + 34952000 + 25046000 + 29947000 + 30054000 },
+        { 29942000, 13573000 + 29969000 + 29958000 + 30054000 + 34952000 + 25046000 + 29947000 + 30054000 + 29942000 },
+};
+
+calculated_number test14b_results[] = {
+        1000000, 1000000, 1000000, 1000000, 1000000, 1000000, 1000000, 1000000
+};
+
+struct test test14b = {
+        "test14b",            // name
+        "issue #981 with dummy data",
+        30,                 // update_every
+        1,                  // multiplier
+        1,                  // divisor
+        RRDDIM_INCREMENTAL, // algorithm
+        10,                 // feed entries
+        8,                  // result entries
+        test14b_feed,        // feed
+        test14b_results,     // results
+        NULL,               // feed2
+        NULL                // results2
+};
+
 // --------------------------------------------------------------------------------------------------------------------
 
 int run_test(struct test *test)
@@ -749,7 +815,7 @@ int run_test(struct test *test)
     snprintfz(name, 100, "unittest-%s", test->name);
 
     // create the chart
-    RRDSET *st = rrdset_create("netdata", name, name, "netdata", NULL, "Unit Testing", "a value", 1, 1, RRDSET_TYPE_LINE);
+    RRDSET *st = rrdset_create("netdata", name, name, "netdata", NULL, "Unit Testing", "a value", 1, test->update_every, RRDSET_TYPE_LINE);
     RRDDIM *rd = rrddim_add(st, "dim1", NULL, test->multiplier, test->divisor, test->algorithm);
     
     RRDDIM *rd2 = NULL;
@@ -759,12 +825,20 @@ int run_test(struct test *test)
     st->debug = 1;
 
     // feed it with the test data
+    time_t time_now = 0, time_start = time(NULL);
     unsigned long c;
+    collected_number last = 0;
     for(c = 0; c < test->feed_entries; c++) {
         if(debug_flags) fprintf(stderr, "\n\n");
 
         if(c) {
-            fprintf(stderr, "    > %s: feeding position %lu, after %llu microseconds\n", test->name, c+1, test->feed[c].microseconds);
+            time_now += test->feed[c].microseconds;
+            fprintf(stderr, "    > %s: feeding position %lu, after %0.3f seconds (%0.3f seconds from start), delta " CALCULATED_NUMBER_FORMAT ", rate " CALCULATED_NUMBER_FORMAT "\n", 
+                test->name, c+1,
+                (float)test->feed[c].microseconds / 1000000.0,
+                (float)time_now / 1000000.0,
+                ((calculated_number)test->feed[c].value - (calculated_number)last) * (calculated_number)test->multiplier / (calculated_number)test->divisor,
+                (((calculated_number)test->feed[c].value - (calculated_number)last) * (calculated_number)test->multiplier / (calculated_number)test->divisor) / (calculated_number)test->feed[c].microseconds * (calculated_number)1000000);
             rrdset_next_usec(st, test->feed[c].microseconds);
         }
         else {
@@ -773,6 +847,7 @@ int run_test(struct test *test)
 
         fprintf(stderr, "       >> %s with value " COLLECTED_NUMBER_FORMAT "\n", rd->name, test->feed[c].value);
         rrddim_set(st, "dim1", test->feed[c].value);
+        last = test->feed[c].value;
 
         if(rd2) {
             fprintf(stderr, "       >> %s with value " COLLECTED_NUMBER_FORMAT "\n", rd2->name, test->feed2[c]);
@@ -785,6 +860,7 @@ int run_test(struct test *test)
         if(!c) {
             fprintf(stderr, "    > %s: fixing first collection time to be %llu microseconds to second boundary\n", test->name, test->feed[c].microseconds);
             rd->last_collected_time.tv_usec = st->last_collected_time.tv_usec = st->last_updated.tv_usec = test->feed[c].microseconds;
+            // time_start = st->last_collected_time.tv_sec;
         }
     }
 
@@ -801,14 +877,21 @@ int run_test(struct test *test)
         calculated_number v = unpack_storage_number(rd->values[c]);
         calculated_number n = test->results[c];
         int same = (roundl(v * 10000000.0) == roundl(n * 10000000.0))?1:0;
-        fprintf(stderr, "    %s/%s: checking position %lu, expecting value " CALCULATED_NUMBER_FORMAT ", found " CALCULATED_NUMBER_FORMAT ", %s\n", test->name, rd->name, c+1, n, v, (same)?"OK":"### E R R O R ###");
+        fprintf(stderr, "    %s/%s: checking position %lu (at %lu secs), expecting value " CALCULATED_NUMBER_FORMAT ", found " CALCULATED_NUMBER_FORMAT ", %s\n",
+            test->name, rd->name, c+1,
+            (rrdset_first_entry_t(st) + c * st->update_every) - time_start,
+            n, v, (same)?"OK":"### E R R O R ###");
+
         if(!same) errors++;
 
         if(rd2) {
             v = unpack_storage_number(rd2->values[c]);
             n = test->results2[c];
             same = (roundl(v * 10000000.0) == roundl(n * 10000000.0))?1:0;
-            fprintf(stderr, "    %s/%s: checking position %lu, expecting value " CALCULATED_NUMBER_FORMAT ", found " CALCULATED_NUMBER_FORMAT ", %s\n", test->name, rd2->name, c+1, n, v, (same)?"OK":"### E R R O R ###");
+            fprintf(stderr, "    %s/%s: checking position %lu (at %lu secs), expecting value " CALCULATED_NUMBER_FORMAT ", found " CALCULATED_NUMBER_FORMAT ", %s\n",
+                test->name, rd2->name, c+1,
+                (rrdset_first_entry_t(st) + c * st->update_every) - time_start,
+                n, v, (same)?"OK":"### E R R O R ###");
             if(!same) errors++;
         }
     }
@@ -857,6 +940,12 @@ int run_all_mockup_tests(void)
     if(run_test(&test13))
         return 1;
 
+    if(run_test(&test14))
+        return 1;
+
+    if(run_test(&test14b))
+        return 1;
+
     return 0;
 }