HPCC-29657 Produce aggregate stats (e.g. spill, cost) whilst a job is running #18048

shamser · 2023-11-17T12:42:00Z

… running

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

jakesmith

@shamser - please see comments.

I'm not sure the structure is quite right.
I think StatsAggregator should be moved into jstats, and more of the aggregation functionality should be encapsulated in it, including recordStats and refreshAggregates.
With only those methods that rely on a IConstWorkUnit/IWorkUnit implemented workunit.*

thorlcr/master/thdemonserver.cpp

common/workunit/workunit.hpp

jakesmith · 2023-11-20T14:14:07Z

thorlcr/master/thdemonserver.cpp

+        reportGraph(statsBuilder, & graph);
+        // Merge only the stats at the specified scope level
+        Linked<IStatisticCollection> statsCollection = statsBuilder.getResult();
+        statsAggregator.recordStats(statsCollection, wfid, graphName, graph.queryGraphId());


Seems a shame to pass in { wfid, grpahName, graph.queryGraphId() }, to relocate the IStatisticCollection (inside recordStats), when updatteStaats has preprepared the gatherer at the correct scope (before reportGraph is called).
Could add a IStatisticCollection *IStatisiticGather::getCurrentScopeCollection(), to get the sgStatsCollection directly, and pass that in.
Perhaps recordStats should also get the scope chain from the collection rather than pass it here (would need something similar to getFullScope, but returned list of scope ids

updateStats creates gatherer at the graph scope not sg scope. Thinking about it, it should be possible to achieve the recordStats without the path.

ecl/eclagent/eclagent.ipp

system/jlib/jstats.cpp

ecl/eclagent/agentctx.hpp

jakesmith · 2023-11-20T19:17:06Z

ecl/eclagent/eclgraph.cpp

@@ -879,10 +879,11 @@ void EclSubGraph::updateProgress()
        Owned<IWUGraphStats> progress = parent.updateStats(queryStatisticsComponentType(), queryStatisticsComponentName(), parent.queryWfid(), id);
        IStatisticGatherer & stats = progress->queryStatsBuilder();
        updateProgress(stats);
-
+        agent->mergeAggregatorStats(stats, parent.queryWfid(), parent.queryGraphName(), id);


see other comments. I think with other changes, this could be something like:

IStatisticGatherer & stats = progress->queryStatsBuilder(); Owned<IStatisticCollection> sgStatsCollection = stats.getCurrentScopeCollection(); cur.updateProgress(stats); agent->mergeAggregatorStats(*sgStatsCollection);

That may not be true for roxie "subgraph" stats which are for a whole graph.

I think as per the other comments, if the "copy stats" method in the StatisticAggregator is generic and is passed an arbitrary IStatisticCollection, then uses that to discover its source scope + calculate the target to copy to, it should be able to be generic code which works in the roxie case too (with roxie similarly passing through its builder current scope level).

jakesmith · 2023-11-20T19:20:08Z

system/jlib/jstats.cpp

+        isDirty=true;
+        if (parent) parent->markDirty();
+    }
+    void refreshAggregates(CRuntimeStatisticCollection & parentTotals, std::function<void(const char * scope, StatisticScopeType sst, StatisticKind kind, stat_type value)> & fWhenAggregateUpdated)


related to other comments.
I think this can and should be moved outside of CStatisticCollection and into the StatisticAggregator class, and for it to be implemented using IStatisticCollection methods.
As per other comments, IStatisticCollection would still need to expose some methods, but it would be separate out the aggregate implementation, and should be cleaner.

jakesmith · 2023-11-20T19:23:11Z

system/jlib/jstats.cpp

@@ -1915,15 +1995,33 @@ class CStatisticCollection : public CInterfaceOf<IStatisticCollection>
        for (iter.first(); iter.isValid(); iter.next())
            iter.query().getMinMaxActivity(minValue, maxValue);
    }
+    virtual bool setStatistic(const char *scope, StatisticKind kind, unsigned __int64 value) override


this is essentially a 'ensureScope' function + a scope->setStatisitic(kind, value); implementation.
It would be cleaner (and enable other changes) if it was separated out to:

IStatisticCollecton::ensureScope(const char *fullScope) = 0; void setStatistic(StatisticKind kind, unsigned __int64 value) = 0;

Although at that point, setStatistic, is very much like updateStatistic, so it may be better to just expose updateStatistic and have the aggregator class call it directly.

jakesmith · 2023-11-20T19:23:37Z

system/jlib/jstats.cpp

    virtual void visitChildren(IStatisticVisitor & visitor) const
    {
        for (auto const & cur : children)
            cur.visit(visitor);
    }
+    virtual void refreshAggregates(const StatisticsMapping & mapping, std::function<void(const char * scope, StatisticScopeType sst, StatisticKind kind, stat_type value)> & fWhenAggregateUpdated) override


could be encapsulated in StatisticAggregator class.

ghalliday

That is much easier to understand, and functionality is better encapsulated. A few comments, most minor, but I don't think the disk cost aggregation would currently be working (uninitialised variable).

One thing to discuss: The impact of moving file costs from the global stats to the subgraph stats.

One potential issue that I have spotted, but should be addressed in a later PR.
This code is assuming the only file costs and execution costs are in the graphs. However, if a query contains file copy/spray/despray operations then they should be recorded against the workflow item.

That suggests that the stats aggregator should possible record a new stats variant of the accumulated costs, rather than reusing the same stats value for the aggregated value.

ghalliday · 2023-11-21T09:29:52Z

ecl/eclagent/eclgraph.cpp

+    Owned<IStatisticCollection> statsCollection = progress.getResult();
+    const cost_type costDiskAccess = statsCollection->aggregateStatistic(StCostFileAccess);
+    if (costDiskAccess)
+        progress.addStatistic(StCostFileAccess, costDiskAccess);


This change means that the cost will be recorded in the subgraphs stats rather than the global stats as it was before. Does that mean it will not be picked up by the stats aggregator?
If you cannot restart a graph I don't think it makes a difference, but worth discussing - it may make a difference for eclwatch responsiveness.

the aggregated total should still be published at the global level after this change (unless I'm missing something),
but instead of it being calculated and set directly into global stats, it is calculated and set indirectly via the statsAggregator (inside updateProgress).

Previously, this statistic wasn't picked up by the aggregator. It needs to be in the subgraph scope. However, the aggregator will add the statistic back in as a global statistic.

ghalliday · 2023-11-21T09:34:10Z

system/jlib/jstats.cpp

+            if (cur.kind == kind)
+            {
+                value += cur.value;
+                found = true;


Can this return true at this point? I don't think there can be more than once instance of a stat.

See further comments about deleting this function.

No, it can't return at this point. It needs to continue summing as there may be duplicate kinds in the stats array.

What can cause multiple entries for the same kind in the array? That sounds like a bug.

ghalliday · 2023-11-21T09:40:58Z

system/jlib/jstats.cpp

+        ForEachItemIn(i, curSrcCollection->stats)
+        {
+            Statistic & cur = curSrcCollection->stats.element(i);
+            if (cur.kind != (StatisticKind)(cur.kind & StKindMask))


easier to read as:

if (queryStatsVariant(cur.kind) != 0)

ghalliday · 2023-11-21T09:44:09Z

system/jlib/jstats.cpp

+    virtual stat_type aggregateStatistic(StatisticKind kind) const override
+    {
+        stat_type sum;
+        if (!getStatisticSum(kind, sum)) // get sum of statistics at this level


This could call getStatistic instead() and avoid the need for a new function.
Actually the existing code is wrong because sum is not initialised on entry - even more reason to change it.

getStatistic returns the first value for the kind. There could be multiple kinds in the stats array. I missed initialization of sum.

See question/comment elsewhere. There should only be a single entry for each kind in the stats collection at each level.

Given there should never be duplicates (and https://track.hpccsystems.com/browse/HPCC-30949 to fix place where could be), the new getStatisticSum() method should now be removed from this PR, and getStatistic() used here instead.

ghalliday · 2023-11-21T09:49:13Z

thorlcr/master/thdemonserver.cpp

+        IStatisticGatherer & statsBuilder = stats->queryStatsBuilder();
+        reportGraph(statsBuilder, & graph);
+        // Merge only the stats at the specified scope level
+        Linked<IStatisticCollection> statsCollection = statsBuilder.getResult();


trivial: I don't think you need to assign to a Linked ptr

I agree with Jake

ghalliday · 2023-11-21T09:56:19Z

system/jlib/jstats.cpp

+            ++scopeItem;
+        }
+
+        CStatisticCollection * tgtScopeCollection = ensureSubScopePath(path);


Something to note as unusual. The source stats have a root item of a workflow item, the target have a root of the global scope. I suspect the subgraph stats should be changed, but snot something to consider at this point.

ghalliday · 2023-11-21T09:58:46Z

system/jlib/jstats.cpp

+    }
+    virtual stat_type aggregateStatistic(StatisticKind kind) const override
+    {
+        stat_type sum;


missing initialiser (both both true and false branches).

ghalliday · 2023-11-21T09:59:50Z

thorlcr/master/thdemonserver.cpp

+    {
+        Owned<IWUGraphStats> stats = currentWU.updateStats(graphName, SCTthor, queryStatisticsComponentName(), wfid, graph.queryGraphId(), false);
+        IStatisticGatherer & statsBuilder = stats->queryStatsBuilder();
+        reportGraph(statsBuilder, & graph);


trivial: extra space before graph.

ghalliday · 2023-11-21T10:13:59Z

system/jlib/jstats.cpp

+            {
+                Statistic & stat = stats.element(i);
+                StatisticKind kind = stat.queryKind();
+                if (kind != (StatisticKind)(kind & StKindMask))


Easier to read as

queryStatsVariant(kind) != 0

(mentioned elsewhere). Possibly better would be to move that test into hasKind()

ghalliday · 2023-11-21T12:15:19Z

I agree with many of Jake's comments - e.g. the aggregator living in jstats and the location of several of the functions. They might be better to address in a separate PR though. I think the structure is now broadly correct, and once it works if may be better to merge as-is.
(It might be better for someone else to refactor the code, and then for Shamser to review it, rather than Shamser trying to second-guess what the reviewers are trying to get at. Something to discuss more generally I suspect.)

jakesmith · 2023-11-22T10:13:24Z

I agree with many of Jake's comments - e.g. the aggregator living in jstats and the location of several of the functions. They might be better to address in a separate PR though. I think the structure is now broadly correct, and once it works if may be better to merge as-is. (It might be better for someone else to refactor the code, and then for Shamser to review it, rather than Shamser trying to second-guess what the reviewers are trying to get at. Something to discuss more generally I suspect.)

@shamser - ok, with this in mind, let's avoid significant refactoring/changes for now in this PR, and focus on the more minor issues/fixes, with the aim of merging this soon. Then we can revisit after that.

jakesmith · 2023-11-22T10:54:17Z

system/jlib/jstats.h

    virtual void serialize(MemoryBuffer & out) const = 0;
    virtual unsigned __int64 queryWhenCreated() const = 0;
    virtual void mergeInto(IStatisticGatherer & target) const = 0;
    virtual StringBuffer &toXML(StringBuffer &out) const = 0;
    virtual void visit(IStatisticVisitor & target) const = 0;
    virtual void visitChildren(IStatisticVisitor & target) const = 0;
+    virtual void refreshAggregates(const StatisticsMapping & mapping, std::function<void(const char * scope, StatisticScopeType sst, StatisticKind kind, stat_type value)> & fWhenAggregateUpdated) = 0;


This callback definition (std::function<void(const char * scope, StatisticScopeType sst, StatisticKind kind, stat_type value)>) would benefit from a typedef to clarify.

shamser · 2023-11-23T11:08:48Z

@jakesmith I've addressed most of the minor suggestions. @ghalliday suggested that it may be easier for you to refactor.

jakesmith · 2023-11-23T13:14:15Z

{quote}
One potential issue that I have spotted, but should be addressed in a later PR.
This code is assuming the only file costs and execution costs are in the graphs. However, if a query contains file copy/spray/despray operations then they should be recorded against the workflow item.

That suggests that the stats aggregator should possible record a new stats variant of the accumulated costs, rather than reusing the same stats value for the aggregated value.
{quote}

@shamser - not sure if a JIRA been opened for this yet? Can you reference here and link as a related issue to this JIRA?

jakesmith

@shamser changes look good I think.

I will make a final pass after @ghalliday has reviewed changes based on his previous comments.

ghalliday

@shamser please can you reply to the question about duplicate stats. Otherwise looks ok to merge.

ghalliday · 2023-11-27T12:36:27Z

system/jlib/jstats.cpp

+    virtual stat_type aggregateStatistic(StatisticKind kind) const override
+    {
+        stat_type sum;
+        if (!getStatisticSum(kind, sum)) // get sum of statistics at this level


See question/comment elsewhere. There should only be a single entry for each kind in the stats collection at each level.

ghalliday · 2023-11-27T12:38:34Z

system/jlib/jstats.cpp

+            if (cur.kind == kind)
+            {
+                value += cur.value;
+                found = true;


What can cause multiple entries for the same kind in the array? That sounds like a bug.

shamser · 2023-11-28T17:14:53Z

RE:See question/comment elsewhere. There should only be a single entry for each kind in the stats collection at each level.
RE: What can cause multiple entries for the same kind in the array? That sounds like a bug.

I think the roxie code uses addStatistic. I didn't delve too deeply into the code to investigate if it is possible for the same kind to exist multiple times. As there are no comments to suggest otherwise and CStatisticCollection allows multiple stats of the same kind to be added, I have had assume that same kind multiple times is supported.

I'm going to run some tests to see if addStatistic is ever used to create the same kind multiple times in CStatisticCollection - I'll post the result here tomorrows. @ghalliday

shamser · 2023-11-29T09:21:29Z

RE:See question/comment elsewhere. There should only be a single entry for each kind in the stats collection at each level. RE: What can cause multiple entries for the same kind in the array? That sounds like a bug.

I think the roxie code uses addStatistic. I didn't delve too deeply into the code to investigate if it is possible for the same kind to exist multiple times. As there are no comments to suggest otherwise and CStatisticCollection allows multiple stats of the same kind to be added, I have had assume that same kind multiple times is supported.

I'm going to run some tests to see if addStatistic is ever used to create the same kind multiple times in CStatisticCollection - I'll post the result here tomorrows. @ghalliday

It seems that Roxie creates multiple stats for the same kind. Please see regression results for this PR: #18090. In this PR, I added an assert to make sure the same kind wasn't add multiple times.
The assert was triggered multiple times. @ghalliday @jakesmith

ghalliday · 2023-11-30T12:45:05Z

RE:See question/comment elsewhere. There should only be a single entry for each kind in the stats collection at each level. RE: What can cause multiple entries for the same kind in the array? That sounds like a bug.
I think the roxie code uses addStatistic. I didn't delve too deeply into the code to investigate if it is possible for the same kind to exist multiple times. As there are no comments to suggest otherwise and CStatisticCollection allows multiple stats of the same kind to be added, I have had assume that same kind multiple times is supported.
I'm going to run some tests to see if addStatistic is ever used to create the same kind multiple times in CStatisticCollection - I'll post the result here tomorrows. @ghalliday

It seems that Roxie creates multiple stats for the same kind. Please see regression results for this PR: #18090. In this PR, I added an assert to make sure the same kind wasn't add multiple times. The assert was triggered multiple times. @ghalliday @jakesmith

Thanks - that is very useful, and should be fixed as a separate issue.

jakesmith

@shamser - please see comments

jakesmith · 2023-12-01T11:29:38Z

common/workunit/workunit.cpp

@@ -2659,83 +2659,67 @@ cost_type aggregateCost(const IConstWorkUnit * wu, const char *scope, bool exclu
    }
 }

-//aggregate disk costs from top-level subgraphs (when scope specified) or workflows (scope not specified)
-cost_type aggregateDiskAccessCost(const IConstWorkUnit * wu, const char *scope)
+void StatisticsAggregator::loadExistingAggregates(IConstWorkUnit &workunit)


void StatisticsAggregator::loadExistingAggregates(IConstWorkUnit &workunit)

picky: IConstWorkUnit could be a const IConstWorkUnit

jakesmith · 2023-12-01T11:38:29Z

common/workunit/workunit.cpp

+        wu->setStatistic(queryStatisticsComponentType(), queryStatisticsComponentName(), sst, scope, kind, nullptr, value, 1, 0, StatsMergeReplace);
+    };
+
+    statsCollection->refreshAggregates(mapping, f);


see: https://github.com/hpcc-systems/HPCC-Platform/actions/runs/6968880782/job/18963669852
Failing to compile in Windows.

The std::function vs 'std::function<void(const char * scope, StatisticScopeType sst, StatisticKind kind, stat_type value)>' I think.

It would be better if the callback function type was a typedef defined in jstats.h, and then used here.

jakesmith · 2023-12-01T11:44:34Z

system/jlib/jstats.cpp

+    virtual stat_type aggregateStatistic(StatisticKind kind) const override
+    {
+        stat_type sum;
+        if (!getStatisticSum(kind, sum)) // get sum of statistics at this level


Given there should never be duplicates (and https://track.hpccsystems.com/browse/HPCC-30949 to fix place where could be), the new getStatisticSum() method should now be removed from this PR, and getStatistic() used here instead.

shamser · 2023-12-01T16:30:38Z

@jakesmith The changes related to the last comments have been merged to the last commit.

ghalliday

I didn't spot any problems.
My only concern is moving the cost from the global stats to the subgraph stats and whether that breaks any wudetails processing. I will look at that separately.

shamser · 2023-12-04T17:21:22Z

I didn't spot any problems. My only concern is moving the cost from the global stats to the subgraph stats and whether that breaks any wudetails processing. I will look at that separately.

The aggregator will create the same cost aggregates as before in the global stats.

ghalliday · 2023-12-04T17:36:24Z

I didn't spot any problems. My only concern is moving the cost from the global stats to the subgraph stats and whether that breaks any wudetails processing. I will look at that separately.

The aggregator will create the same cost aggregates as before in the global stats.

But it will not create a cost for a subgraph in the global stats, which is the difference.

jakesmith

@shamser - looks good, please squash.

… running Signed-off-by: Shamser Ahmed <[email protected]>

jakesmith

@shamser - looks good.

shamser force-pushed the issue29657new2 branch from 628bd2f to e90b81f Compare November 17, 2023 14:41

shamser changed the title ~~HPCC-29657 Produce aggregate stats (e.g. spill, cost) whilst a job is…~~ HPCC-29657 Produce aggregate stats (e.g. spill, cost) whilst a job is running Nov 17, 2023

shamser force-pushed the issue29657new2 branch 2 times, most recently from 05dece3 to eddf198 Compare November 17, 2023 17:52

shamser marked this pull request as ready for review November 17, 2023 17:54

shamser force-pushed the issue29657new2 branch 3 times, most recently from 118afc7 to b663a50 Compare November 17, 2023 18:04

shamser changed the base branch from master to candidate-9.4.x November 17, 2023 18:04

shamser force-pushed the issue29657new2 branch 2 times, most recently from 2768811 to ed6acf6 Compare November 17, 2023 18:13

shamser requested review from jakesmith and ghalliday November 17, 2023 18:14

jakesmith reviewed Nov 20, 2023

View reviewed changes

ghalliday reviewed Nov 21, 2023

View reviewed changes

jakesmith reviewed Nov 22, 2023

View reviewed changes

shamser requested a review from jakesmith November 23, 2023 11:08

jakesmith reviewed Nov 23, 2023

View reviewed changes

jakesmith requested a review from ghalliday November 23, 2023 13:16

ghalliday reviewed Nov 27, 2023

View reviewed changes

shamser requested review from ghalliday and jakesmith November 29, 2023 13:27

jakesmith requested changes Dec 1, 2023

View reviewed changes

shamser force-pushed the issue29657new2 branch 3 times, most recently from 17dcaea to 73b7bcd Compare December 1, 2023 16:26

shamser requested a review from jakesmith December 1, 2023 16:30

ghalliday approved these changes Dec 4, 2023

View reviewed changes

jakesmith approved these changes Dec 4, 2023

View reviewed changes

HPCC-29657 Produce aggregate stats (e.g. spill, cost) whilst a job is…

d45f744

… running Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue29657new2 branch from 73b7bcd to d45f744 Compare December 5, 2023 11:04

shamser requested a review from jakesmith December 5, 2023 11:08

jakesmith approved these changes Dec 5, 2023

View reviewed changes

ghalliday changed the base branch from candidate-9.4.x to master December 7, 2023 16:44

ghalliday merged commit 2a959b9 into hpcc-systems:master Dec 7, 2023
45 checks passed

HPCC-29657 Produce aggregate stats (e.g. spill, cost) whilst a job is running #18048

HPCC-29657 Produce aggregate stats (e.g. spill, cost) whilst a job is running #18048

Conversation

shamser commented Nov 17, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Nov 21, 2023

jakesmith commented Nov 22, 2023

Choose a reason for hiding this comment

shamser commented Nov 23, 2023

jakesmith commented Nov 23, 2023

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser commented Nov 28, 2023 • edited Loading

shamser commented Nov 29, 2023 • edited Loading

ghalliday commented Nov 30, 2023

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser commented Dec 1, 2023

ghalliday left a comment

Choose a reason for hiding this comment

shamser commented Dec 4, 2023

ghalliday commented Dec 4, 2023

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

shamser commented Nov 17, 2023 •

edited

Loading

shamser commented Nov 28, 2023 •

edited

Loading

shamser commented Nov 29, 2023 •

edited

Loading