From c961b46b7fc0f0dbee0817aa384351adab7239dc Mon Sep 17 00:00:00 2001 From: Norbert Hartl Date: Mon, 21 Sep 2026 12:26:30 +0200 Subject: [PATCH] A marked method is not the only one, and a leaf that raises is not the scrape Two things the exporter and the builder did quietly, both found in the review of 2026-09-20 and both reproduced in image [a] before being touched. buildComplexProperties ran the marked method only when there was exactly one of them - no else, no complaint. A class with two methods therefore got neither, and "two" is not exotic: it is what a subclass has the moment it adds a branch to the one it inherits. Measured on the old code with an example that declares two: both keys missing, not one. Every marked method is sent now, each once, in pragma order. It is sent by the method's own selector rather than by #canopyBranch: derived from the pragma name. Identical for all ten declarations in the image - checked, the only one named otherwise is the new example - but only one of the two answers the question "which method did I mark". An override appears twice in the pragma list and both entries name the same selector, so distinct selectors run the override once and never the method it replaces. visitMetric: evaluated a leaf and let whatever came out of it leave through #format:. One component that cannot answer therefore ended the export where it stood, and every series after it left the scrape too - which reads as a server that stopped reporting rather than as one value that could not be read. On the old code the whole export raised. The guard sits on the ask, because asking is what evaluates: once the holder is in hand the readings are too, so the stream cannot be left carrying a header whose lines never came. The review note said the filter sat one level too late, and that framing was wrong: it cannot move earlier without giving up the model where the returned holder decides what a leaf contributes. What was wrong was the blast radius, and that is what changed. Canopy-Core-Tests 91 passed in image [a], the same four stale failures as before - none of those four tests exists on main. Co-Authored-By: Claude Opus 5 (1M context) --- .../CanopyAccessorTest.class.st | 11 ++++++++ .../CanopyRaisingLeafExample.class.st | 19 ++++++++++++++ .../CanopyTwoBranchExample.class.st | 26 +++++++++++++++++++ .../Canopy-Core/CanopyMappingBuilder.class.st | 26 ++++++++++++------- .../CanopyMetricsVisitorTest.class.st | 25 ++++++++++++++++++ .../CanopyPrometheusVisitor.class.st | 16 ++++++++++-- 6 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 source/Canopy-Core-Tests/CanopyRaisingLeafExample.class.st create mode 100644 source/Canopy-Core-Tests/CanopyTwoBranchExample.class.st diff --git a/source/Canopy-Core-Tests/CanopyAccessorTest.class.st b/source/Canopy-Core-Tests/CanopyAccessorTest.class.st index 4ac15e6..662139f 100644 --- a/source/Canopy-Core-Tests/CanopyAccessorTest.class.st +++ b/source/Canopy-Core-Tests/CanopyAccessorTest.class.st @@ -184,3 +184,14 @@ CanopyAccessorTest >> testTwoAccessorsThatDeclareNothingStillConflict [ one case the repeat-tolerance above must not swallow." self should: [ CanopyAccessor new merge: CanopyAccessor new ] raise: Error ] + +{ #category : 'as yet unclassified' } +CanopyAccessorTest >> testEveryBranchMethodContributes [ + "Two methods carrying and both of them build. Until 2026-09-21 the builder + ran the marked method only when there was exactly one, so a second one did not add a + branch - it removed the first, silently and without an else." + | tree | + tree := CanopyTwoBranchExample new canopyMapping. + self assert: (tree / #first) value equals: 1. + self assert: (tree / #second) value equals: 2 +] diff --git a/source/Canopy-Core-Tests/CanopyRaisingLeafExample.class.st b/source/Canopy-Core-Tests/CanopyRaisingLeafExample.class.st new file mode 100644 index 0000000..b606073 --- /dev/null +++ b/source/Canopy-Core-Tests/CanopyRaisingLeafExample.class.st @@ -0,0 +1,19 @@ +" +A component whose reader raises, which is what a live one does on a bad day - +the servers read their values out of blocks, and AGSoilMetrics reduces over every +open database while databases open and close underneath it. + +Here to hold the exporter to its promise that such a leaf costs that leaf and not +the whole scrape. +" +Class { + #name : 'CanopyRaisingLeafExample', + #superclass : 'Object', + #category : 'Canopy-Core-Tests', + #package : 'Canopy-Core-Tests' +} + +{ #category : 'accessing' } +CanopyRaisingLeafExample >> brokenReading [ + ^ self error: 'this component cannot answer today' +] diff --git a/source/Canopy-Core-Tests/CanopyTwoBranchExample.class.st b/source/Canopy-Core-Tests/CanopyTwoBranchExample.class.st new file mode 100644 index 0000000..8b1af97 --- /dev/null +++ b/source/Canopy-Core-Tests/CanopyTwoBranchExample.class.st @@ -0,0 +1,26 @@ +" +An object that marks two methods with , which until 2026-09-21 was +a way to end up with no composed tree at all: the builder ran the marked method +only when there was exactly one of them, and said nothing otherwise. + +Two rather than one is not exotic. It is what a subclass gets the moment it adds +a branch of its own to the one it inherits. +" +Class { + #name : 'CanopyTwoBranchExample', + #superclass : 'Object', + #category : 'Canopy-Core-Tests', + #package : 'Canopy-Core-Tests' +} + +{ #category : 'building' } +CanopyTwoBranchExample >> canopyBranch: aBuilder [ + + aBuilder map at: #first put: (CanopyCell new value: 1) +] + +{ #category : 'building' } +CanopyTwoBranchExample >> canopySecondBranch: aBuilder [ + + aBuilder map at: #second put: (CanopyCell new value: 2) +] diff --git a/source/Canopy-Core/CanopyMappingBuilder.class.st b/source/Canopy-Core/CanopyMappingBuilder.class.st index de9d2bb..878d688 100644 --- a/source/Canopy-Core/CanopyMappingBuilder.class.st +++ b/source/Canopy-Core/CanopyMappingBuilder.class.st @@ -38,15 +38,23 @@ CanopyMappingBuilder >> build [ { #category : 'as yet unclassified' } CanopyMappingBuilder >> buildComplexProperties [ - | pragmas | - pragmas := Pragma - allNamed: #canopyBranch - from: object class - to: Object. - (pragmas size = 1) ifTrue: [ - object - perform: pragmas first selector asMutator - with: self ] + "Every method that carries the marker, each sent once, in the order the pragmas come in. + + This used to run only when there was exactly one and do nothing at all otherwise, with no + else and no complaint: a subclass that added a branch method of its own lost the one it + inherited as well, and the whole composed half of the tree simply was not there. + + Sent by the method's own selector rather than by #canopyBranch: derived from the pragma + name. They are the same for every declaration there is today, but only one of the two is + an answer to the question 'which method did I mark'. An override is in the list twice, + the subclass's and the superclass's, and both name that same selector - so taking distinct + selectors runs the override once and never the method it replaces." + | selectors | + selectors := OrderedCollection new. + (Pragma allNamed: #canopyBranch from: object class to: Object) do: [ :each | + (selectors includes: each methodSelector) ifFalse: [ + selectors add: each methodSelector ] ]. + selectors do: [ :each | object perform: each with: self ] ] { #category : 'as yet unclassified' } diff --git a/source/Canopy-Metrics-Tests/CanopyMetricsVisitorTest.class.st b/source/Canopy-Metrics-Tests/CanopyMetricsVisitorTest.class.st index f3672dd..5be607b 100644 --- a/source/Canopy-Metrics-Tests/CanopyMetricsVisitorTest.class.st +++ b/source/Canopy-Metrics-Tests/CanopyMetricsVisitorTest.class.st @@ -290,3 +290,28 @@ CanopyMetricsVisitorTest >> testExportWithoutPrefixWritesNameAsIs [ self assert: (output includesSubstring: 'apptiveGrid_memorySize'). self deny: (output includesSubstring: 'pharo') ] + +{ #category : 'as yet unclassified' } +CanopyMetricsVisitorTest >> testALeafThatRaisesCostsThatLeafAndNotTheScrape [ + "Reading a leaf is a live call into a component, and one that fails used to end the export + where it stood: the error left #format: altogether, so the answer was not a short scrape but + no scrape - which looks like a server that stopped reporting rather than like one value that + could not be read. + + Which of the two the tree hands the visitor first does not matter, and the test does not + depend on it: the old code raised out of #format: whichever way round they came." + | tree output | + tree := CanopyBranchNode new. + tree at: #brokenReading put: (CanopyAccessor new + object: CanopyRaisingLeafExample new; + readSelector: #brokenReading; + yourself). + tree at: #workingReading put: (CanopyAccessor reading: [ + CanopyMetric new + type: #gauge; + description: 'a reading that answers'; + value: 42 ]). + output := CanopyPrometheusVisitor new format: tree. + self deny: (output includesSubstring: 'brokenReading'). + self assert: (output includesSubstring: 'workingReading 42 ') +] diff --git a/source/Canopy-Metrics/CanopyPrometheusVisitor.class.st b/source/Canopy-Metrics/CanopyPrometheusVisitor.class.st index ff48cc5..2dfc3d4 100644 --- a/source/Canopy-Metrics/CanopyPrometheusVisitor.class.st +++ b/source/Canopy-Metrics/CanopyPrometheusVisitor.class.st @@ -79,9 +79,21 @@ CanopyPrometheusVisitor >> visitCell: aCanopyCell [ CanopyPrometheusVisitor >> visitMetric: aNode [ "Ask the value what it contributes and write one line per reading. A value that is not a metric answers nothing, so nothing is written - not even the HELP and TYPE header. That is - why a config value in an exported tree now costs no output instead of ending the scrape." + why a config value in an exported tree now costs no output instead of ending the scrape. + + And a leaf that raises while it is read costs that leaf rather than the scrape. Reading is + a live call into a component - the servers' values are blocks, AGSoilMetrics reduces over + every open database - and one of them failing used to end the export where it stood, so + everything after it in the tree silently left the scrape as well. Nothing is written for it + now, which reads as a gap in one series instead of a flat line in all of them. + + Asking is what evaluates, so the guard sits on the ask rather than around the writing: once + the holder is in hand the readings are too, and the stream cannot be left carrying a header + whose lines never came." | holder header | - holder := aNode valueHolder. + holder := [ aNode valueHolder ] + on: Error + do: [ :error | ^ self ]. header := false. holder readingsDo: [ :labels :value | header ifFalse: [