Make pool statistics optional #453

smallhive · 2023-06-20T11:15:00Z

close #431

roman-khimov

What I propose is to think more in line of #431 (comment) and #431 (comment)

It may look like an additional SetOperationCallback() for Client's (!) PrmInit. In the form of f func(nodekey []byte, endpoint string, method string, t time.Duration, err error). Endpoint is to be taken from Dial() parameters while nodekey is to be extracted from EndpointInfo() (which can be called instead of a Balance() in Client.Dial(). The rest of the things are pretty obvious, method names should be specified in the documentation.

The reason for endpoint/key is that there can be multiple nodes behind one address and we need to deal with it. This also makes the interface generic enough to be reused in exactly the same way in Pool.InitParameters.

Then there is going to be a stats package with a structure implementing this method (Statistic can be moved there), it has to be protected for concurrent access though. This package could be used both for client and pool. But node could reuse the same interface for its (kludgy) reputationClient (counting things it wants to count).

Then pool has to manage the health of its clients. Always. It has some knobs for it (like ErrorThreshold, but it's an internal pool thing, always there, always passed to clients. If it also has an externally-requested OperationCallback() then it's also called (kinda a proxied call). SetRequestCallback() can be removed, because it's the same thing, just with less parameters.

I think this scheme will cover all of our use cases and allow for some more.

pool/pool.go

roman-khimov

But looks OK otherwise.

stat/stat.go

client/accounting.go

client/client.go

roman-khimov · 2023-06-22T20:49:55Z

pool/pool.go

+	case methodContainerAnnounceUsedSpace:
+		return "containerAnnounceUsedSpace"
+	case methodSyncContainerWithNetwork:
+		return "syncContainerWithNetwork"


These will be removed eventually.

Ok, I will remove it from stat

I mean from the pool! All of the real client method should be covered. But syncContainer is not a proper method at all.

Still there, container sync is not a client method.

Yes, it uses the client but not it's part. Removed

roman-khimov · 2023-06-22T20:50:54Z

pool/pool.go

 	return res, nil
 }

+func (c *clientWrapper) statisticMiddleware(nodeKey []byte, endpoint string, method stat.Method, duration time.Duration, err error) {
+	c.incRequests(duration, MethodIndex(method))


This one won't be needed after default statistics decoupling from the pool.

Still there and we still have Statistics which can be reworked into a stat thing (some structure that exposes the same interface to retrieve statistics and implements OperationCallback).

Moved to stat package

roman-khimov · 2023-06-29T19:03:45Z

stat/stat.go

+	MethodNetMapSnapshot
+	MethodObjectHash
+	MethodObjectSearch
+	methodLast


Do we have all client methods here? It should cover all of them even though the pool doesn't use some.

Method list is updated, now we have all client methods

roman-khimov · 2023-06-29T19:04:47Z

client/accounting.go

 		res accounting.Decimal
+		cc  contextCall


Not a useful change (moving var definition).

Change removed

roman-khimov · 2023-06-29T19:10:20Z

stat/stat.go

@@ -28,6 +28,7 @@ const (
 	MethodObjectSearch
 	MethodContainerAnnounceUsedSpace
 	MethodSyncContainerWithNetwork
+	// methodLast is no a valid method name, it's a system anchor for tests, etc.


This comment is added in a separate commit from methodLast definition.

roman-khimov · 2023-06-29T19:11:28Z

client/object_search.go

 	}
 	r.client = c
+	r.statisticCallback = func(duration time.Duration, err error) {
+		c.sendStatistic(stat.MethodObjectSearch, duration, err)


Is it counted like a MethodObjectSearch now in pool? I'd expect some separate name since it's not a real method, rather just reading from the stream.

Same with get/put.

Extended with three new Method*

roman-khimov · 2023-06-29T19:13:48Z

client/client.go

-	_, err := rpc.Balance(&c.c, new(v2accounting.BalanceRequest),
-		client.WithContext(prm.parentCtx),
-	)
+	endpointInfo, err := c.EndpointInfo(prm.parentCtx, PrmEndpointInfo{})
 	// return context errors since they signal about dial problem
 	if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {


So why do we still have these then?

roman-khimov · 2023-06-29T19:17:19Z

pool/pool.go

 	return res, nil
 }

+func (c *clientWrapper) statisticMiddleware(nodeKey []byte, endpoint string, method stat.Method, duration time.Duration, err error) {
+	c.incRequests(duration, MethodIndex(method))


Still there and we still have Statistics which can be reworked into a stat thing (some structure that exposes the same interface to retrieve statistics and implements OperationCallback).

roman-khimov · 2023-06-29T19:17:54Z

pool/pool.go

+	case methodContainerAnnounceUsedSpace:
+		return "containerAnnounceUsedSpace"
+	case methodSyncContainerWithNetwork:
+		return "syncContainerWithNetwork"


Still there, container sync is not a client method.

stat/stat.go

client/client.go

pool/pool.go

client/accounting.go

stat/stat.go

stat/monitor.go

carpawell · 2023-07-03T18:23:13Z

Also, a general suggestion: add more details to commit messages. Some examples from node: nspcc-dev/neofs-node@00afc57, nspcc-dev/neofs-node@67bd9c2.

Changes are good, but the knowledge of why they were done allows speeding up reading and understanding.

smallhive · 2023-07-04T06:21:32Z

Also, a general suggestion: add more details to commit messages. Some examples from node: nspcc-dev/neofs-node@00afc57, nspcc-dev/neofs-node@67bd9c2.

Changes are good, but the knowledge of why they were done allows speeding up reading and understanding.

It makes sense

stat/stat.go

client/accounting.go

stat/stat.go

stat/pool.go

Using EndpointInfo, instead of Balance have two advantages: - Check the entrypoint is available; - Get node public key for statistic purposes. Any error now is a reason to return from the constructor and broke initialization. Signed-off-by: Evgenii Baidakov <[email protected]>

Signed-off-by: Evgenii Baidakov <[email protected]>

…mands Signed-off-by: Evgenii Baidakov <[email protected]>

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive force-pushed the 431-make-pool-statistics-optional branch 3 times, most recently from 473b938 to bf18054 Compare June 20, 2023 11:30

smallhive marked this pull request as ready for review June 20, 2023 12:04

smallhive requested review from roman-khimov, cthulhu-rider and notimetoname as code owners June 20, 2023 12:04

roman-khimov reviewed Jun 21, 2023

View reviewed changes

pool/pool.go Outdated Show resolved Hide resolved

pool/pool.go Outdated Show resolved Hide resolved

pool/pool.go Outdated Show resolved Hide resolved

smallhive force-pushed the 431-make-pool-statistics-optional branch from bf18054 to 8f4c375 Compare June 22, 2023 10:28

smallhive marked this pull request as draft June 22, 2023 10:28

smallhive force-pushed the 431-make-pool-statistics-optional branch 2 times, most recently from d5cea25 to 6ec5b2a Compare June 22, 2023 12:38

roman-khimov reviewed Jun 22, 2023

View reviewed changes

smallhive force-pushed the 431-make-pool-statistics-optional branch 5 times, most recently from 5620e71 to f2512e0 Compare June 26, 2023 09:22

smallhive marked this pull request as ready for review June 26, 2023 09:49

smallhive requested a review from roman-khimov June 26, 2023 09:49

roman-khimov reviewed Jun 29, 2023

View reviewed changes

smallhive force-pushed the 431-make-pool-statistics-optional branch 6 times, most recently from 2bae95b to a55bfbc Compare July 3, 2023 04:43

smallhive requested a review from roman-khimov July 3, 2023 04:46

smallhive force-pushed the 431-make-pool-statistics-optional branch from a55bfbc to 3ac0b0b Compare July 3, 2023 05:54

carpawell reviewed Jul 3, 2023

View reviewed changes

stat/stat.go Show resolved Hide resolved

client/client.go Show resolved Hide resolved

pool/pool.go Show resolved Hide resolved

client/accounting.go Outdated Show resolved Hide resolved

stat/stat.go Show resolved Hide resolved

stat/monitor.go Show resolved Hide resolved

smallhive force-pushed the 431-make-pool-statistics-optional branch 2 times, most recently from 9fab285 to 1dabbc3 Compare July 4, 2023 06:19

smallhive requested a review from carpawell July 4, 2023 06:23

carpawell approved these changes Jul 5, 2023

View reviewed changes

carpawell removed the request for review from notimetoname July 5, 2023 14:53

cthulhu-rider approved these changes Jul 6, 2023

View reviewed changes

stat/stat.go Show resolved Hide resolved

stat/stat.go Show resolved Hide resolved

client/accounting.go Show resolved Hide resolved

stat/stat.go Show resolved Hide resolved

stat/pool.go Show resolved Hide resolved

smallhive added 10 commits July 6, 2023 09:09

client: External statistic in client

17efd8c

Signed-off-by: Evgenii Baidakov <[email protected]>

client: External statistic tests

8cf10a2

Signed-off-by: Evgenii Baidakov <[email protected]>

pool: Remove unused statisticUpdater

549a23d

Signed-off-by: Evgenii Baidakov <[email protected]>

stat: Separate exec commands stats with stream operation in these com…

a270918

…mands Signed-off-by: Evgenii Baidakov <[email protected]>

stat: Extract methods statistic from pool

95e6055

Signed-off-by: Evgenii Baidakov <[email protected]>

stat: Statistic concurrency tests

94a129e

Signed-off-by: Evgenii Baidakov <[email protected]>

pool: Remove poolRequestInfoCallback and incRequests func

c29c89a

Signed-off-by: Evgenii Baidakov <[email protected]>

pool: Remove elapsedTimeCallback

d5782d6

Signed-off-by: Evgenii Baidakov <[email protected]>

pool: Remove old statistic method enum

41bd8e5

Signed-off-by: Evgenii Baidakov <[email protected]>

smallhive force-pushed the 431-make-pool-statistics-optional branch from 1dabbc3 to 41bd8e5 Compare July 6, 2023 05:10

smallhive requested a review from carpawell July 6, 2023 05:12

notimetoname approved these changes Jul 6, 2023

View reviewed changes

cthulhu-rider merged commit 6acff6a into master Jul 6, 2023
5 checks passed

cthulhu-rider deleted the 431-make-pool-statistics-optional branch July 6, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make pool statistics optional #453

Make pool statistics optional #453

smallhive commented Jun 20, 2023

roman-khimov left a comment •

edited

Loading

roman-khimov left a comment

roman-khimov Jun 22, 2023

smallhive Jun 23, 2023

roman-khimov Jun 23, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 22, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 29, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 29, 2023

smallhive Jun 30, 2023

roman-khimov Jun 29, 2023

roman-khimov Jun 29, 2023

carpawell commented Jul 3, 2023

smallhive commented Jul 4, 2023

Make pool statistics optional #453

Make pool statistics optional #453

Conversation

smallhive commented Jun 20, 2023

roman-khimov left a comment • edited Loading

Choose a reason for hiding this comment

roman-khimov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Jul 3, 2023

smallhive commented Jul 4, 2023

roman-khimov left a comment •

edited

Loading