HPCC-29790 Switch to using > as a stage name prefix #17483

ghalliday · 2023-06-22T15:14:51Z

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:

github-actions · 2023-06-22T15:15:22Z

https://track.hpccsystems.com/browse/HPCC-29790
Jira updated

ghalliday · 2023-06-22T15:17:20Z

@GordonSmith here is my experimental PR.
Note the change in the TS - I'm not sure if that is where you would place it.
Note, the labels for containers (pseudo-subgraphs) do not have the > removed. I didn't know where to modify.
It also has a change from another PR (adding git timings) and a few fixes for scopes.

Main questions are what are the implications of adding the prefix, will it cause any problems, and what needs changing to improve the display?

GordonSmith · 2023-06-27T08:37:04Z

esp/src/src-react/util/metricGraph.ts

@@ -203,7 +203,10 @@ export class MetricGraph extends Graph2<IScope, IScopeEdge, IScope> {
    }

    vertexLabel(v: IScope, options: MetricsOptions): string {
-        return v.type === "activity" ? format(options.activityTpl, v) : v.Label || v.id;
+        return v.type === "activity" ? format(options.activityTpl, v) :
+               v.type === "function" ? v.id + "()" :


Does "function" have a "Label"?

GordonSmith · 2023-06-27T08:37:16Z

esp/src/src-react/util/metricGraph.ts

-        return v.type === "activity" ? format(options.activityTpl, v) : v.Label || v.id;
+        return v.type === "activity" ? format(options.activityTpl, v) :
+               v.type === "function" ? v.id + "()" :
+               v.type === "stage" && v.id.charAt(0) === '>' ? v.id.substring(1) :


Does "stage" have a "Label"?

ghalliday · 2024-01-05T14:42:53Z

@GordonSmith @shamser This is pushed and rebased for discussion.
The problem it is trying to solve is that we often have arbitrary text "e.g., compile, generate++, copy, import, etc. etc." which is used to indicate a scope. Most scopes can deduce the scope type from the scope text. E.g. graph10, ac23,
sg99, fmyFunc.
The idea is to use a prefix character - in this case > to indicate that the scope is an operation. However this has a few problems as far as the UI goes - all the labels and display text look strange. Is the correct solution to filter any ">"s from the text before displaying it?
A different alternative would be that if you cannot recognise the scope text then assume it is an operation. However that has problems that you couldn't have any operations beginning with f, n or p. (These are prefixes for function, section and file scope). I don't think file is currently used and could be changed. But whatever prefix is used should probably be stripped in a similar way.
This has been on my list to fix for a while, and @shamser needs it to integrate the file copy stats properly into the workunits.
Please think about it, and may be best to have a meeting to talk about it next week.

shamser

I think that representing "compile" in SSToperation is going to be challenging. Both clarity and bug prevention will be hard to achieve.

SSToperation is prefixing all operations with >. However, "compile" scope is used with SSToperation type without the prefix. I would imagine there is there potential for bugs. There is definitely one issue where a StatsScopeId may set to SSToperation("compile") but calling getScopeText on the same StatsScopeId would return ">compile".

I believe SSTcompilestage should remain to represent the top level compile scope.

shamser · 2024-01-05T15:59:52Z

ecl/hql/hqlexpr.cpp

-        scope.append("compiler::parse::").append(name);
-        parseCtx.statsTarget.addStatistic(SSTcompilestage, scope, StTimeTotalExecute, nullptr, totalTime, 1, 0, StatsMergeSum);
-        parseCtx.statsTarget.addStatistic(SSTcompilestage, scope, StTimeLocalExecute, nullptr, localTime, 1, 0, StatsMergeSum);
+        scope.append("compile::>parse::>").append(name);


May be unrelated to this but why is double colons used here?

A mistake... They should be single.

shamser · 2024-01-05T16:52:08Z

system/jlib/jstatcodes.h

@@ -66,7 +68,7 @@ enum StatisticScopeType
    SSTactivity,
    SSTallocator,                       // identifies an allocator
    SSTsection,                         // A section within the query - not a great differentiator
-    SSTcompilestage,                    // a stage within the compilation process
+    SSToperation,                       // an operation or stage in processing


I believe there is a good case to have "compile" as the parent scope for compilation related stats. And if "compile" is a valid scope, then SSTcompilestage should remain. So, for "compile:>compile c++", it makes sense to use SSTcompilestage(compile) and SSToperation(compile c++).

StatsScopeId::getScopeText() is called when scopeType is SSToperation and name is "compile" should it return ">compile" or would there be special code to return "compile"?

(Or, SSTcompilestage is eliminated then ">compile" should be used instead of "compile" in all cases. Instead of "compile:>compile c++", the following format should be used if SSTcompilestage is eliminated ">compile:>compile c++". )

A good point. I started off using >compile everywhere, and then relaxed it, but you are correct it will be inconsistent, which is not great. It could be also be a SSTstage where it only matches specific words with no prefix - if so would there be any other examples it could be used for? Your "dfu" scope could be another.

SSTstage sounds like a good idea. I agree it could be used for dfu as well. I assume there would be a prefix for stage?

shamser · 2024-01-05T16:56:50Z

common/workunit/workunit.cpp

@@ -12346,7 +12346,7 @@ extern WORKUNIT_API void submitWorkUnit(const char *wuid, const char *username,

    {
        Owned<IWorkUnit> wu = &cw->lock();
-        addTimeStamp(wu, SSTcompilestage, "compile", StWhenQueued, 0);
+        addTimeStamp(wu, SSToperation, "compile", StWhenQueued, 0);


Shouldn't all SSToperation be prefixed with >? Shouldn't it be either

addTimeStamp(wu, SSTcompilestage, "compile", StWhenQueued, 0);

OR
addTimeStamp(wu, SSToperation, ">compile", StWhenQueued, 0);

shamser · 2024-01-05T16:59:30Z

system/jlib/jstats.cpp

@@ -554,7 +554,7 @@ StatisticScopeType getScopeType(const char * scope)
    if (nullptr == scope)
        id.setId(SSTglobal, 0);
    else if (startsWith(scope, "compile"))
-        id.setId(SSTcompilestage, 0);
+        id.setId(SSToperation, 0);


This produces the scope ">". Line 556-557 may be eliminated as setScopeText in the next code block (line 560-564) has code to handle compile scope.

shamser · 2024-01-05T17:02:14Z

system/jlib/jstats.cpp

+    case SSTsection:
+        return out.append(SectionScopePrefix).append(name);
+    case SSToperation:
+        return out.append(OperationScopePrefix).append(name);


Note, all operation scopes are prefixed with >. It wouldn't ever produce the scope "compile" but ">compile". Please see comment regarding keeping SSTcompilestage.

shamser · 2024-01-05T17:03:47Z

system/jlib/jstats.cpp

@@ -1673,6 +1686,11 @@ bool StatsScopeId::setScopeText(const char * text, const char * * _next)
            setChildGraphId(strtoul(text+ strlen(ChildGraphScopePrefix), next, 10));
            return true;
        }
+        if (MATCHES_CONST_PREFIX(text, "compile"))
+        {
+            setOperationId("compile");


Note, getScopeText for this will return ">compile"

shamser

Looks good.

ghalliday · 2024-01-19T17:33:18Z

@GordonSmith please can we talk this through next week to check what implications it has for eclwatch

Signed-off-by: Gavin Halliday <[email protected]>

ghalliday requested a review from GordonSmith June 22, 2023 15:14

GordonSmith reviewed Jun 27, 2023

View reviewed changes

ghalliday mentioned this pull request Jan 3, 2024

HPCC-31048 Fix the scope for SSTdfuworkunit #18175

Merged

39 tasks

ghalliday force-pushed the issue29790 branch from 88f19cc to 8d44f60 Compare January 4, 2024 10:39

ghalliday changed the base branch from candidate-9.2.x to master January 4, 2024 10:39

ghalliday force-pushed the issue29790 branch 2 times, most recently from 71d3b51 to 38b8271 Compare January 5, 2024 14:30

ghalliday marked this pull request as ready for review January 5, 2024 14:31

ghalliday requested review from GordonSmith and shamser January 5, 2024 14:31

ghalliday force-pushed the issue29790 branch from 38b8271 to 57cfe27 Compare January 5, 2024 14:44

shamser requested changes Jan 5, 2024

View reviewed changes

ghalliday force-pushed the issue29790 branch 2 times, most recently from bb90e7b to 95f5c4e Compare January 19, 2024 13:49

shamser approved these changes Jan 19, 2024

View reviewed changes

ghalliday force-pushed the issue29790 branch from 19892a9 to 6800628 Compare January 25, 2024 10:46

HPCC-29790 Use '>' as a prefix for stage names in stats scopes

7a190fb

Signed-off-by: Gavin Halliday <[email protected]>

ghalliday force-pushed the issue29790 branch from 6800628 to 7a190fb Compare January 25, 2024 10:48

ghalliday merged commit cb7016f into hpcc-systems:master Jan 25, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-29790 Switch to using > as a stage name prefix #17483

HPCC-29790 Switch to using > as a stage name prefix #17483

ghalliday commented Jun 22, 2023 •

edited

Loading

github-actions bot commented Jun 22, 2023

ghalliday commented Jun 22, 2023

GordonSmith Jun 27, 2023

GordonSmith Jun 27, 2023

ghalliday commented Jan 5, 2024

shamser left a comment

shamser Jan 5, 2024

ghalliday Jan 9, 2024

shamser Jan 5, 2024

ghalliday Jan 9, 2024

shamser Jan 9, 2024

shamser Jan 5, 2024

shamser Jan 5, 2024

shamser Jan 5, 2024

shamser Jan 5, 2024

shamser left a comment

ghalliday commented Jan 19, 2024

HPCC-29790 Switch to using > as a stage name prefix #17483

HPCC-29790 Switch to using > as a stage name prefix #17483

Conversation

ghalliday commented Jun 22, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jun 22, 2023

ghalliday commented Jun 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Jan 5, 2024

shamser 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

shamser left a comment

Choose a reason for hiding this comment

ghalliday commented Jan 19, 2024

ghalliday commented Jun 22, 2023 •

edited

Loading