HPCC-30588 Publish descriptions for each of the statistic types #18299

ghalliday · 2024-02-15T11:11:52Z

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 · 2024-02-15T11:12:09Z

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

ghalliday · 2024-02-15T13:10:31Z

Test by using the url:
http://:8010/WsWorkunits/WuDetailsMeta.json

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

ghalliday · 2024-02-15T13:47:54Z

@JamesDeFabia please check the language
@GordonSmith please check this provides what you need. I'll add some details to the jira.
@jakesmith please check everything!

GordonSmith · 2024-02-15T13:51:41Z

system/jlib/jstats.cpp

+    { SIZESTAT(MaxRowSize), "The high water mark of the memory used for representing rows (roxiemem)" },
+    { NUMSTAT(RowsProcessed), "How many rows have been processed" },
+    { NUMSTAT(Slaves), "The number of parallel execution processes used to execute an activity" },
+    { NUMSTAT(Starts), "How many times the activity has started executing\nAn activity is active if this does not match NumStops" },


Minor: missing period prior to new line?

An activity is active if this does not match NumStop

not sure worth expanding on, but mismatch might indicate an abnormal termination state vs active.

GordonSmith · 2024-02-15T13:54:58Z

system/jlib/jstats.cpp

+    { PERSTAT(Replicated), "The percentage replication complete" },
+    { NUMSTAT(DiskRowsRead), "The number of rows read from the file" },
+    { NUMSTAT(IndexRowsRead), "The number of rows read from the index" },
+    { NUMSTAT(DiskAccepted), "The number of disk rows that resturn a result from the transform" },


Typos resturn

GordonSmith · 2024-02-15T13:56:16Z

system/jlib/jstats.cpp

+    { TIMESTAT(SpillElapsed), "Time spent spilling rows from memory to disk" }, //MORE: Do we have a similar stat for SpillRead?
+    { TIMESTAT(SortElapsed), "Time spent sorting rows in memory" },
+    { NUMSTAT(Groups), "The number of groups processed by this activity" },
+    { NUMSTAT(GroupMax), "The size of the largest group processed by this activity.\nA skew in group size can cause a skew in processing time.  A large skew may indicate some special values would benefit from special casing." },


Minor: period at end of line is inconsistent.

GordonSmith · 2024-02-15T13:57:04Z

system/jlib/jstats.cpp

+    { NUMSTAT(ScansPerRow), UNUSED },
+    { NUMSTAT(Allocations), "The number of allocations from the row memory" },
+    { NUMSTAT(AllocationScans), "The number of scans within the memory manager when allocating row memory\nOnly applies to the scanning heap manager (not used by default)" },
+    { NUMSTAT(DiskRetries), "The number of times an io operation was retried.\nIf this is non zero it may suggest a problem with the underlying disk storage" },


io -> IO (Similar to OS)?

GordonSmith · 2024-02-15T13:58:30Z

system/jlib/jstats.cpp

+    { NUMSTAT(SysContextSwitches), "The number of context switches that occurred when processing" },
+    { TIMESTAT(OsUser), "Total elapsed user-space time" },
+    { TIMESTAT(OsSystem), "Total time spent in the system/kernel" },
+    { TIMESTAT(OsTotal), "Total elapsed time according to the OS.\nIncludes system,user,idle and iowait times" },


Minor: spaces after commas?

GordonSmith · 2024-02-15T14:06:53Z

system/jlib/jstats.cpp

+    { WHENFIRSTSTAT(Dequeued), "The time when this item was removed from a queue" },
+    { WHENFIRSTSTAT(K8sLaunched), "The time when the job to procss this item was launched" },
+    { WHENFIRSTSTAT(K8sStarted), "The time when the job to procss this item started executing/nThe difference between the K8sStarted and K8sLaunched indicates how long Kubernetes took to resource and initialised the job." },
+    { WHENFIRSTSTAT(K8sReady), "The time when the Thor job is ready to process\nThe difference with K8sStarted inidcates how long it took to resource and start the slave processes" },


Missing period before newline.

GordonSmith · 2024-02-15T14:07:38Z

system/jlib/jstats.cpp

+    { SIZESTAT(RowMemory), "The size of memory used to store rows" },
+    { SIZESTAT(PeakRowMemory), "The peak memory used to store rows" },
+    { SIZESTAT(AgentSend), "The size of data sent to the agent from the server" },
+    { TIMESTAT(IndexCacheBlocked), "The time spend waiting to access the index page cache" },


spend -> spent?

GordonSmith · 2024-02-15T14:07:48Z

system/jlib/jstats.cpp

    { CYCLESTAT(IndexCacheBlocked) },
-    { TIMESTAT(AgentProcess) },
+    { TIMESTAT(AgentProcess), "The total time spend by the agents processing requests" },


spend -> spent?

GordonSmith · 2024-02-15T14:08:18Z

system/jlib/jstats.cpp

-    { SIZESTAT(ContinuationData) },
-    { NUMSTAT(ContinuationRequests) },
+    { NUMSTAT(AckRetries), "How many times the server fails to receive a response from an agent within the expected time" },
+    { SIZESTAT(ContinuationData), "The total size of continuation data sent from agent to the server\nA large number may indicate a poor filter, or merging from many different index locations" },


Missing period before newline.

GordonSmith · 2024-02-15T14:08:52Z

system/jlib/jstats.cpp

-    { NUMSTAT(ContinuationRequests) },
+    { NUMSTAT(AckRetries), "How many times the server fails to receive a response from an agent within the expected time" },
+    { SIZESTAT(ContinuationData), "The total size of continuation data sent from agent to the server\nA large number may indicate a poor filter, or merging from many different index locations" },
+    { NUMSTAT(ContinuationRequests), "The number of time the agent indicated there was more data to be returned" },


time -> times

JamesDeFabia

A few comments inline. NB: I only reviewed the language in jstats.cpp.

JamesDeFabia · 2024-02-15T14:06:53Z

system/jlib/jstats.cpp

+    { SIZESTAT(GeneratedCpp), "The size of the generated c++ file" },
+    { SIZESTAT(PeakMemory), "The peak memory used while processing this item" },
+    { SIZESTAT(MaxRowSize), "The high water mark of the memory used for representing rows (roxiemem)" },
+    { NUMSTAT(RowsProcessed), "How many rows have been processed" },


For consistency: The number of rows processed

JamesDeFabia · 2024-02-15T14:08:01Z

system/jlib/jstats.cpp

+    { SIZESTAT(MaxRowSize), "The high water mark of the memory used for representing rows (roxiemem)" },
+    { NUMSTAT(RowsProcessed), "How many rows have been processed" },
+    { NUMSTAT(Slaves), "The number of parallel execution processes used to execute an activity" },
+    { NUMSTAT(Starts), "How many times the activity has started executing\nAn activity is active if this does not match NumStops" },


For consistency: The number of times the activity has started executing...

JamesDeFabia · 2024-02-15T14:08:46Z

system/jlib/jstats.cpp

+    { NUMSTAT(RowsProcessed), "How many rows have been processed" },
+    { NUMSTAT(Slaves), "The number of parallel execution processes used to execute an activity" },
+    { NUMSTAT(Starts), "How many times the activity has started executing\nAn activity is active if this does not match NumStops" },
+    { NUMSTAT(Stops), "How many times the activity has stopped executing.\nAn activity is active if this is less than NumStarts" },


For consistency: The number of times the activity has stopped executing.

JamesDeFabia · 2024-02-15T14:11:36Z

system/jlib/jstats.cpp

+    { NUMSTAT(PreloadCacheHits), UNUSED },
+    { NUMSTAT(PreloadCacheAdds), UNUSED },
+    { NUMSTAT(ServerCacheHits), UNUSED },
+    { NUMSTAT(IndexAccepted), "The number of keyed join matches that return a result from the transform" },


Keywords s/b All CAPS: KEYED JOIN TRANSFORM

JamesDeFabia · 2024-02-15T14:11:55Z

system/jlib/jstats.cpp

+    { NUMSTAT(PreloadCacheAdds), UNUSED },
+    { NUMSTAT(ServerCacheHits), UNUSED },
+    { NUMSTAT(IndexAccepted), "The number of keyed join matches that return a result from the transform" },
+    { NUMSTAT(IndexRejected), "The number of keyed join matches that are skipped by the transform" },


Keywords s/b All CAPS: KEYED JOIN TRANSFORM

JamesDeFabia · 2024-02-15T14:13:22Z

system/jlib/jstats.cpp

-    { SIZESTAT(SpillFile) },
+    { NUMSTAT(DiskReads), "The number of disk read operations" },
+    { NUMSTAT(DiskWrites), "The number of disk write operations" },
+    { NUMSTAT(Spills), "How many times the activity spilt to disk"},


For consistency: The number of times the activity spilt to disk

JamesDeFabia · 2024-02-15T14:14:52Z

system/jlib/jstats.cpp

+    { NUMSTAT(ScansPerRow), UNUSED },
+    { NUMSTAT(Allocations), "The number of allocations from the row memory" },
+    { NUMSTAT(AllocationScans), "The number of scans within the memory manager when allocating row memory\nOnly applies to the scanning heap manager (not used by default)" },
+    { NUMSTAT(DiskRetries), "The number of times an io operation was retried.\nIf this is non zero it may suggest a problem with the underlying disk storage" },


JamesDeFabia · 2024-02-15T14:16:07Z

system/jlib/jstats.cpp

-    { COSTSTAT(Execute) },
-    { SIZESTAT(AgentReply) },
-    { TIMESTAT(AgentWait) },
+    { COSTSTAT(Execute), "Cpu cost of executing" },


Cpu s/b CPU

JamesDeFabia · 2024-02-15T14:16:37Z

system/jlib/jstats.cpp

-    { COSTSTAT(Compile) },
-    { TIMESTAT(NodeLoad) },
+    { COSTSTAT(FileAccess), "The transactional cost of any file operations" },
+    { NUMSTAT(Pods), "How many pods were used" },


For consistency: The number of pods used.

JamesDeFabia · 2024-02-15T14:18:32Z

system/jlib/jstats.cpp

-    { NUMSTAT(AckRetries) },
-    { SIZESTAT(ContinuationData) },
-    { NUMSTAT(ContinuationRequests) },
+    { NUMSTAT(AckRetries), "How many times the server fails to receive a response from an agent within the expected time" },


For consistency: The number of times...

jakesmith

@ghalliday - looks good.

Some pedantic issues mostly, the present/past-tense of the sentences are a bit inconsistent some of the time - I think it would be better if always past tense.

jakesmith · 2024-02-16T10:51:22Z

system/jlib/jstats.cpp

+    { WHENFIRSTSTAT(Created), "The time when an item was created" },
+    { WHENFIRSTSTAT(Compiled), "The time a workunit started being compiled" },
+    { WHENFIRSTSTAT(WorkunitModified), UNUSED },
+    { TIMESTAT(Elapsed), "The elapsed time between starting and finishing\nFor child queries this may be significantly larger than TimeTotalExecute" },


missing full stop before \n for consistency.

But for ultimate consistency, all single line descriptions should also have a terminating full stop.

I think I was aiming for no fullstops since these are likely to appear in tooltips (so I would remove the ones above). @GordonSmith what would you prefer?

Primarily consistency, I don't really mind either way.

jakesmith · 2024-02-16T10:52:46Z

system/jlib/jstats.cpp

+    { WHENFIRSTSTAT(Compiled), "The time a workunit started being compiled" },
+    { WHENFIRSTSTAT(WorkunitModified), UNUSED },
+    { TIMESTAT(Elapsed), "The elapsed time between starting and finishing\nFor child queries this may be significantly larger than TimeTotalExecute" },
+    { TIMESTAT(LocalExecute), "The time spent executing this activity not including its inputs\nSort activities by local execute time to help isolate potential processing bottlenecks" },


missing full stop before \n

jakesmith · 2024-02-16T10:54:36Z

system/jlib/jstats.cpp

+    { SIZESTAT(MaxRowSize), "The high water mark of the memory used for representing rows (roxiemem)" },
+    { NUMSTAT(RowsProcessed), "How many rows have been processed" },
+    { NUMSTAT(Slaves), "The number of parallel execution processes used to execute an activity" },
+    { NUMSTAT(Starts), "How many times the activity has started executing\nAn activity is active if this does not match NumStops" },


An activity is active if this does not match NumStop

not sure worth expanding on, but mismatch might indicate an abnormal termination state vs active.

jakesmith · 2024-02-16T10:56:11Z

system/jlib/jstats.cpp

+    { NUMSTAT(IndexScans), "The number of index scans.\nHow many entries are sequentially examined after an initial seek (including wild seeks).  Large numbers compared to the number of seeks may indicate extra keyed filters would be worthwhile" },
+    { NUMSTAT(IndexWildSeeks), "The number of seeks caused by WILD() filters.\nThe number of keyed lookups that had to search for the next potential match.  If this is a high proportion of NumIndexScans it may suggest poor key design" },
+    { NUMSTAT(IndexSkips), "The number of smart-stepping operations that increment the next match" },
+    { NUMSTAT(IndexNullSkips), "The number of smart-stepping operations that have no effect.\nIf this is large compare to NumIndexSkips it suggests the priority may not be set correctly" },


that have

that had?

jakesmith · 2024-02-16T10:58:53Z

system/jlib/jstats.cpp

+    { NUMSTAT(PostFiltered), "The number of index matches filtered by the transform and the non-keyed filter" },
+    { NUMSTAT(BlobCacheHits), "The number of times a blob is resolved in the cache" },
+    { NUMSTAT(LeafCacheHits), "The number of times a leaf node is resolved in the cache" },
+    { NUMSTAT(NodeCacheHits), "The number of times a branch node is resolved in the cache" },


tense: "is resolved" -> "was resolved"?

NB: below it is past tense : "was read"

jakesmith · 2024-02-16T11:14:22Z

system/jlib/jstats.cpp

+    { NUMSTAT(AttribsSimplified), UNUSED },
+    { NUMSTAT(AttribsFromCache), UNUSED },
+    { NUMSTAT(SmartJoinDegradedToLocal), "The number of times a global smart-join switched to a LOCAL JOIN (with distribute)" },
+    { NUMSTAT(SmartJoinSlavesDegradedToStd), "The number of times a global smart-join degraded to a standard join" },


is it worth saying, after newline. That the above 2 stats will either be 0 or 1, unless in a LOOP?

jakesmith · 2024-02-16T11:16:31Z

system/jlib/jstats.cpp

-    { TIMESTAT(AgentWait) },
+    { COSTSTAT(Execute), "Cpu cost of executing" },
+    { SIZESTAT(AgentReply), "Size of data sent from the workers to the agent" },
+    { TIMESTAT(AgentWait), "Time that the agent spends waiting for a reply from the workers" },


spends->spent ?

jakesmith · 2024-02-16T11:23:57Z

system/jlib/jstats.cpp

    { CYCLESTAT(NodeLoad) },
-    { TIMESTAT(LeafLoad) },
+    { TIMESTAT(LeafLoad), "Time spent reading leaf nodes from disk and decompressing them\nIf this is a high proportion of the time (especially compared to TimeLeadRead) then consider using the new index compression formats" },


TimeLeadRead -> TimeLeafRead

jakesmith · 2024-02-16T11:28:44Z

system/jlib/jstats.cpp

+const char * queryStatisticDescription(StatisticKind kind)
+{
+    StatisticKind rawkind = (StatisticKind)(kind & StKindMask);
+    if (rawkind >= StKindNone && rawkind < StMax)


result will be same I think either way, but should this be rawKind > StKindNone ?

It is consistent with the other functions.

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

ghalliday

Made changes (and a few others). I have removed all periods, but I can add them back if that is preferred.

ghalliday · 2024-02-16T12:12:41Z

system/jlib/jstats.cpp

+    { WHENFIRSTSTAT(Created), "The time when an item was created" },
+    { WHENFIRSTSTAT(Compiled), "The time a workunit started being compiled" },
+    { WHENFIRSTSTAT(WorkunitModified), UNUSED },
+    { TIMESTAT(Elapsed), "The elapsed time between starting and finishing\nFor child queries this may be significantly larger than TimeTotalExecute" },


I think I was aiming for no fullstops since these are likely to appear in tooltips (so I would remove the ones above). @GordonSmith what would you prefer?

ghalliday · 2024-02-16T12:23:15Z

system/jlib/jstats.cpp

+const char * queryStatisticDescription(StatisticKind kind)
+{
+    StatisticKind rawkind = (StatisticKind)(kind & StKindMask);
+    if (rawkind >= StKindNone && rawkind < StMax)


It is consistent with the other functions.

JamesDeFabia

A few comments inline

JamesDeFabia · 2024-02-20T13:22:54Z

system/jlib/jstats.cpp

+    { NUMSTAT(IndexWildSeeks), "The number of seeks caused by WILD() filters\nThe number of keyed lookups that had to search for the next potential match.  If this is a high proportion of NumIndexScans it may suggest poor key design" },
+    { NUMSTAT(IndexSkips), "The number of smart-stepping operations that increment the next match" },
+    { NUMSTAT(IndexNullSkips), "The number of smart-stepping operations that had no effect\nIf this is large compare to NumIndexSkips it suggests the priority may not be set correctly" },
+    { NUMSTAT(IndexMerges), "The number of merges set up when smart stepping"},


Consistency: other instances of smart-stepping are hyphenated

system/jlib/jstats.cpp

JamesDeFabia · 2024-02-20T13:24:34Z

system/jlib/jstats.cpp

+    { NUMSTAT(IndexRowsRead), "The number of rows read from the index" },
+    { NUMSTAT(DiskAccepted), "The number of disk rows that return a result from the TRANSFORM" },
+    { NUMSTAT(DiskRejected), "The number of disk rows that are skipped by the TRANSFORM" },
+    { TIMESTAT(Soapcall), "The time taken to executing a SOAPCALL" },


executing s/b execute

JamesDeFabia · 2024-02-20T13:26:56Z

system/jlib/jstats.cpp

+    { NUMSTAT(ScansPerRow), UNUSED },
+    { NUMSTAT(Allocations), "The number of allocations from the row memory" },
+    { NUMSTAT(AllocationScans), "The number of scans within the memory manager when allocating row memory\nOnly applies to the scanning heap manager (not used by default)" },
+    { NUMSTAT(DiskRetries), "The number of times an I/O operation was retried\nIf this is non zero it may suggest a problem with the underlying disk storage" },


non-zero s/b hyphenated

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

JamesDeFabia

Good from my POV. (I accept smart stepping without a hyphen when not used as an adjective. )

HPCC-30588 Publish descriptions for each of the statistic types

a14c681

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

ghalliday force-pushed the issue30588 branch from 053279c to a14c681 Compare February 15, 2024 13:46

ghalliday requested review from JamesDeFabia, jakesmith and GordonSmith February 15, 2024 13:46

GordonSmith requested changes Feb 15, 2024

View reviewed changes

JamesDeFabia requested changes Feb 15, 2024

View reviewed changes

jakesmith reviewed Feb 16, 2024

View reviewed changes

Changes following review

f5c95d8

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

ghalliday commented Feb 16, 2024

View reviewed changes

ghalliday requested review from JamesDeFabia and GordonSmith February 16, 2024 12:24

JamesDeFabia requested changes Feb 20, 2024

View reviewed changes

Further changes

b0f936e

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

ghalliday requested review from JamesDeFabia and GordonSmith and removed request for GordonSmith February 21, 2024 10:45

JamesDeFabia approved these changes Feb 21, 2024

View reviewed changes

ghalliday merged commit b713bbd into hpcc-systems:candidate-9.4.x Feb 22, 2024
48 checks passed

HPCC-30588 Publish descriptions for each of the statistic types #18299

HPCC-30588 Publish descriptions for each of the statistic types #18299

Conversation

ghalliday commented Feb 15, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Feb 15, 2024

ghalliday commented Feb 15, 2024

ghalliday commented Feb 15, 2024

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

JamesDeFabia 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

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

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

JamesDeFabia 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

JamesDeFabia left a comment

Choose a reason for hiding this comment

ghalliday commented Feb 15, 2024 •

edited

Loading