likely related to how we aggregate sum vs count in graphite configuration across retentions (1m:7d,5m:14d,15m:30d,1h:1y)

# Aggregation methods for whisper files.
storage_aggregation => {
    'min'   => {
        pattern           => '\.min$',
        xFilesFactor      => 0.1,
        aggregationMethod => 'min',
    },
    'max'   => {
        pattern           => '\.max$',
        xFilesFactor      => 0.1,
        aggregationMethod => 'max',
    },
    'sum'   => {
        pattern           => '\.count$',
        xFilesFactor      => 0,
        aggregationMethod => 'sum',
    },
    # statsite extended counters
    'lower' => {
        pattern           => '\.lower$',
        xFilesFactor      => 0.1,
        aggregationMethod => 'min',
    },
    'upper' => {
        pattern           => '\.upper$',
        xFilesFactor      => 0.1,
        aggregationMethod => 'max',
    },
},

and statsite calculates these stats for both counters and timers as

STREAM("%s%s.count|%lld|%lld\n", prefix, name, counter_count(value));
STREAM("%s%s.sum|%f|%lld\n", prefix, name, counter_sum(value));

STREAM("%s%s.count|%lld|%lld\n", prefix, name, timer_count(&t->tm));
STREAM("%s%s.sum|%f|%lld\n", prefix, name, timer_sum(&t->tm));

in any case, count is how many samples have been received and sum the sum of the sample values received, both per flush interval (60s). so sum gets aggregated as average over retention archives (i.e. the first time after 7d) while count gets summed over the same periods

this is what I think is going on, does it match expectations? I'm not sure

I do think summing sum's is going to be more logically consistent for aggregation periods

Yes. count and sum both should be aggregated using sum(). They have different meanings (number of data points vs actual data) but in both cases aggregation logic should use addition.

The key of the count aggregation in the storage_aggregation object refers to the sum property. But it was removed in f4be20a148 – which also removed lower and upper. The latter two were then re-added in fbc91d3284, but sum was never re-introduced.

Change 235612 had a related patch set uploaded (by Krinkle):
graphite: Add aggregation logic for ".sum" properties

https://gerrit.wikimedia.org/r/235612

Change 235612 merged by Ori.livneh:
graphite: Add aggregation logic for ".sum" properties

https://gerrit.wikimedia.org/r/235612

for existing archives we'll need to retroactively change archive aggregation, I can take care of that starting next week

Is there something we need to do to deploy https://gerrit.wikimedia.org/r/235612? I don't see any difference in data aggregated in the past 24 hours.

In T111170#1605166, @Krinkle wrote:

Is there something we need to do to deploy https://gerrit.wikimedia.org/r/235612? I don't see any difference in data aggregated in the past 24 hours.

the aggregation periods are set in whisper files and the graphite configuration applies only to newly created whisper files. I can retroactively change existing archives but data older than 7d that's already been aggregated won't change ttbomk, newly aggregated data will however so will see changes as time goes on.

to clarify the above, whisper files (one per metric) are created as needed when new metrics (not data) shows up. thus the settings in https://gerrit.wikimedia.org/r/235612 ATM will apply to all new metrics that flow in, existing metrics like mw.js.deprecate.wgNamespaceNumber.sum have still the old settings because the whisper file has been created already.
we should retroactively change the aggregation for existing whisper files and see the effects when data is aggregated again as time goes by. existing aggregated data will be left in place, it will get eventually replaced by data aggregated using sum. We can try changing only mw.js.deprecate.wgNamespaceNumber.sum and see how that looks like

@fgiunchedi Ah explains the behaviour I've been seeing. The settings are hardcoded when new whisper files are created. Whenever fresh data is aggregated, it uses the settings present in that file.

Yeah, let's change that one as example to verify the patch (which is already merged!) actually works as expected. If it works fine, perhaps we can apply this more broadly? We're driving blind in some areas because of this. The sooner we can start aggregating data correctly the better.

In T111170#1607084, @Krinkle wrote:

@fgiunchedi Ah explains the behaviour I've been seeing. The settings are hardcoded when new whisper files are created. Whenever fresh data is aggregated, it uses the settings present in that file.

Yeah, let's change that one as example to verify the patch (which is already merged!) actually works as expected. If it works fine, perhaps we can apply this more broadly? We're driving blind in some areas because of this. The sooner we can start aggregating data correctly the better.

sounds good!

_graphite@graphite1001:/tmp$ whisper-set-aggregation-method /var/lib/carbon/whisper/mw/js/deprecate/wgNamespaceNumber/sum.wsp sum
Updated aggregation method: /var/lib/carbon/whisper/mw/js/deprecate/wgNamespaceNumber/sum.wsp (average -> sum)

This should do it:

#!/usr/bin/env bash
shopt -s globstar
for wsp in /var/lib/carbon/whisper/**/sum.wsp; do
  if [[ $(/usr/bin/whisper-info "$wsp" aggregationMethod) != "sum" ]]; then
    /usr/bin/whisper-set-aggregation-method "$wsp" sum
  fi
done

@ori that would, before running it we should check whisper-set-aggregation-method locks the whisper file. if it doesn't we risk clashing with carbon-cache

In T111170#1607731, @fgiunchedi wrote:

@ori that would, before running it we should check whisper-set-aggregation-method locks the whisper file. if it doesn't we risk clashing with carbon-cache

The code for locking the file is there:

https://github.com/graphite-project/whisper/blob/master/whisper.py#L283-L284

But it is conditional on the value of LOCK, which is unconditionally set to False:

https://github.com/graphite-project/whisper/blob/master/whisper.py#L84

indeed, IIRC carbon sets whisper.LOCK = True explicitly if gets configured with locking writes, which we do

Hacked this together, seems to work (but did not run it in prod):

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import os

import whisper

assert whisper.CAN_LOCK
whisper.LOCK = True

for root, dirs, files in os.walk('/var/lib/carbon/whisper'):
    for basename in files:
        if basename.endswith('sum.wsp'):
            filename = os.path.join(root, basename)
            try:
                info = whisper.info(filename)
                if info['aggregationMethod'] != 'sum':
                    old_method = whisper.setAggregationMethod(filename, 'sum')
                    print('%s: %s -> sum' % (filename, old_method))
            except Exception as e:
                    print('Fail to update %s: %s' % filename, e)

Seems to work as expected.

Last 7 days (unaggregated):
https://graphite.wikimedia.org/render/?from=-7days&until=-1h&target=legendValue(mw.js.deprecate.wgNamespaceNumber.%7Bcount%2Csum%7D%2C%22max%22)&width=500

Last 8 days (aggregated):
https://graphite.wikimedia.org/render/?from=-8days&until=-1h&target=legendValue(mw.js.deprecate.wgNamespaceNumber.%7Bcount%2Csum%7D%2C%22max%22)&width=500

since the above seems to be working I've ran the script to all mw.js hierarchy, will apply everywhere tomorrow if no issues arise