Issue #449 API endpoint for Peer Monitor metrics #572

kishansagathiya · 2018-10-07T10:20:42Z

Opened new endpoint GET /health/metrics/<name> which would respond
with metrics of type <name>
Support the new endpoint in rest/api/client
Support the new method in ipfs-cluster-ctl. i.e. ipfs-cluster-ctl health metrics would show the peers and the last list of metrics logged for each as returned by the Peer Monitor, in a friendly way.
Tests

Fixes #449

coveralls · 2018-10-07T10:50:55Z

Coverage decreased (-0.3%) to 65.182% when pulling ec4588a on kishansagathiya:issue_449 into b0b826d on ipfs:master.

kishansagathiya · 2018-10-08T03:37:00Z

@hsanjuan @lanzafame
If this looks good to you so far, I can start with tests.

hsanjuan

Thanks! First round of comments.

hsanjuan · 2018-10-16T21:04:01Z

api/rest/client/client.go

+
+	// PeerMonitorLatestMetrics returns a map with the latest metrics of matching name
+	// for the current cluster peers.
+	PeerMonitorLatestMetrics(name string) ([]api.Metric, error)


Suggested change

PeerMonitorLatestMetrics(name string) ([]api.Metric, error)

Metrics(name string) ([]api.Metric, error)

hsanjuan · 2018-10-16T21:04:17Z

api/rest/client/client.go

@@ -103,6 +103,10 @@ type Client interface {
 	// GetConnectGraph returns an ipfs-cluster connection graph.  The
 	// serialized version, strings instead of pids, is returned
 	GetConnectGraph() (api.ConnectGraphSerial, error)
+
+	// PeerMonitorLatestMetrics returns a map with the latest metrics of matching name


Suggested change

// PeerMonitorLatestMetrics returns a map with the latest metrics of matching name

// Metrics returns a map with the latest metrics of matching name

hsanjuan · 2018-10-16T21:06:31Z

api/rest/restapi.go

+		{
+			"PeerMonitorLatestMetrics",
+			"GET",
+			"/health/metrics/{name}",


Let's name this /monitor/metrics/.. because this comes from the monitor component? Or should this be /informer/<name> because it returns stuff from the informer. Hmm I'm not sure. @lanzafame what do you think?

hsanjuan · 2018-10-16T21:07:13Z

api/rest/restapi.go

+		"PeerMonitorLatestMetrics",
+		name,
+		&metrics)
+	sendResponse(w, err, metrics)


will break with #578 . Reminder to rebase on master once that's merged. (or viceversa)

hsanjuan · 2018-10-16T21:09:36Z

cmd/ipfs-cluster-ctl/formatters.go

@@ -202,6 +215,15 @@ func textFormatPrintAddedOutput(obj *api.AddedOutput) {
 	fmt.Printf("added %s %s\n", obj.Cid, obj.Name)
 }

+func textFormatPrintMetric(obj *api.Metric) {
+	fmt.Printf("{\n")
+	fmt.Printf("	Peer:   %s\n", (obj.Peer).String())


Suggested change

fmt.Printf(" Peer: %s\n", (obj.Peer).String())

fmt.Printf(" Peer: %s\n", obj.Peer.Pretty())

hsanjuan · 2018-10-16T21:12:15Z

cmd/ipfs-cluster-ctl/formatters.go

@@ -202,6 +215,15 @@ func textFormatPrintAddedOutput(obj *api.AddedOutput) {
 	fmt.Printf("added %s %s\n", obj.Cid, obj.Name)
 }

+func textFormatPrintMetric(obj *api.Metric) {


This should not be { .. }. This is text. i.e.

<peer> | <metric name>: > Value: <v> > Expire: <date> > Valid: <t>

for consistency with other outputs (peers ls)

On second thoughts, Valid will be always true so we can skip that. We should probably collapse them to a single line:

<peer id>: <value> | Expire: <date> (user should know the name anyway so we skip)

hsanjuan · 2018-10-16T21:12:27Z

cmd/ipfs-cluster-ctl/formatters.go

+	fmt.Printf("	Peer:   %s\n", (obj.Peer).String())
+	fmt.Printf("	Value:  %s\n", obj.Value)
+	fmt.Printf("	Expire: %d\n", obj.Expire)
+	fmt.Printf("	Expire: %t\n", obj.Valid)


s/Expire/Valid

hsanjuan · 2018-10-16T21:14:41Z

cmd/ipfs-cluster-ctl/formatters.go

+	fmt.Printf("{\n")
+	fmt.Printf("	Peer:   %s\n", (obj.Peer).String())
+	fmt.Printf("	Value:  %s\n", obj.Value)
+	fmt.Printf("	Expire: %d\n", obj.Expire)


We do %s for a date in a different place (textFormatPrintGPInfo). I don't know if the result is the same, but we should use the same printing format for dates in both places.

hsanjuan · 2018-10-16T21:15:43Z

cmd/ipfs-cluster-ctl/main.go

@@ -762,6 +762,20 @@ graph of the connections.  Output is a dot file encoding the cluster's connectio
 						return nil
 					},
 				},
+				{
+					Name:  "metrics",
+					Usage: "Get latest metrics of matching name for the current cluster peers.",


Suggested change

Usage: "Get latest metrics of matching name for the current cluster peers.",

Usage: "List latest metrics logged by this peer",

hsanjuan · 2018-10-16T21:17:54Z

cmd/ipfs-cluster-ctl/main.go

+					Name:  "metrics",
+					Usage: "Get latest metrics of matching name for the current cluster peers.",
+					Description: `
+This command would show the peers and the last list of metrics logged for each as


This commands displays the latest valid metrics of the given type logged by this peer for all current cluster peers.

Rename method PeerMonitorLatestMetrics to Metrics Addressing first round of comment as in ipfs-cluster#572 (review) License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Opened new endpoint `GET /health/metrics/<name>` which would respond with metrics of type <name> License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Support the new endpoint for later metrics in `rest/api/client` Support the new method created in `rest/api/client` in ipfs-cluster-ctl. i.e. `ipfs-cluster-ctl health metrics <name>` would show the peers and the last list of metrics logged for each as returned by the Peer Monitor, in a friendly way. License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Rename method PeerMonitorLatestMetrics to Metrics Addressing first round of comment as in ipfs-cluster#572 (review) License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

hsanjuan

Cool, I missed this was ready! Thanks @kishansagathiya

kishansagathiya · 2018-10-23T00:42:51Z

@hsanjuan Wouldn' this change require us to add sharness tests ? My bad, I took too long to update this PR.

hsanjuan · 2018-10-23T01:05:12Z

Hmm, actually it requires API tests, client tests and sharness tests. Can you do another PR? Also, can you create your branches directly in the ipfs-cluster repo/origin? That way I can commit to them and can save some roundtrips.

kishansagathiya · 2018-10-23T01:49:26Z

can you create your branches directly in the ipfs-cluster repo/origin

Sure, will do that

Issue #572 exposes metrics but they carry the peer ID in binary. This was ok with our internal codecs but it doesn't seem to work very well with json, and makes the output format unusable. This makes the Metric.Peer field a string. Additinoally, fixes calling the command without arguments and displaying the date in the right format. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

lanzafame · 2018-10-23T23:23:05Z

api/rest/restapi.go

+	name := vars["name"]
+
+	var metrics []types.Metric
+	err := api.rpcClient.Call("",


@kishansagathiya I am not reopening this pr for this but in the future if multilining a function call do it in the format of a multiline struct initialization, i.e.

err := api.rpcClient.Call( "", "Cluster", "PeerMonitorLatestMetrics", name, &metrics, )

Benefits of doing it this way are 1) new parameters don't result in a multiline git diff but just a single addition, 2) the first parameter doesn't visually blend into the with the function call.

Don't worry about changing it now. This is just for future reference. 🙂

will keep this in mind

Issue #572 exposes metrics but they carry the peer ID in binary. This was ok with our internal codecs but it doesn't seem to work very well with json, and makes the output format unusable. This makes the Metric.Peer field a string. Additinoally, fixes calling the command without arguments and displaying the date in the right format. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ghost assigned kishansagathiya Oct 7, 2018

ghost added the status/in-progress In progress label Oct 7, 2018

kishansagathiya requested review from lanzafame and hsanjuan October 8, 2018 03:35

hsanjuan requested changes Oct 16, 2018

View reviewed changes

kishansagathiya added 3 commits October 21, 2018 12:49

Issue ipfs-cluster#449 API endpoint for Peer Monitor metrics

fafbf7b

Opened new endpoint `GET /health/metrics/<name>` which would respond with metrics of type <name> License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Issue ipfs-cluster#449 API endpoint for Peer Monitor metrics

ec4588a

Rename method PeerMonitorLatestMetrics to Metrics Addressing first round of comment as in ipfs-cluster#572 (review) License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

kishansagathiya force-pushed the issue_449 branch from 16cc58a to ec4588a Compare October 21, 2018 08:06

hsanjuan approved these changes Oct 22, 2018

View reviewed changes

hsanjuan mentioned this pull request Oct 22, 2018

Release 0.7.0 #571

Closed

hsanjuan merged commit 48c89fb into ipfs-cluster:master Oct 22, 2018

ghost removed the status/in-progress In progress label Oct 22, 2018

kishansagathiya mentioned this pull request Oct 23, 2018

Tests for /monitor/metrics/<name> endpoint #587

Closed

hsanjuan mentioned this pull request Oct 23, 2018

Make metrics human #589

Merged

lanzafame reviewed Oct 23, 2018

View reviewed changes

kishansagathiya deleted the issue_449 branch November 6, 2019 15:38

kishansagathiya restored the issue_449 branch November 6, 2019 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #449 API endpoint for Peer Monitor metrics #572

Issue #449 API endpoint for Peer Monitor metrics #572

kishansagathiya commented Oct 7, 2018 •

edited

coveralls commented Oct 7, 2018 •

edited

kishansagathiya commented Oct 8, 2018

hsanjuan left a comment

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan Oct 16, 2018

hsanjuan left a comment

kishansagathiya commented Oct 23, 2018

hsanjuan commented Oct 23, 2018

kishansagathiya commented Oct 23, 2018

lanzafame Oct 23, 2018

kishansagathiya Oct 24, 2018

	PeerMonitorLatestMetrics(name string) ([]api.Metric, error)
	Metrics(name string) ([]api.Metric, error)

	// PeerMonitorLatestMetrics returns a map with the latest metrics of matching name
	// Metrics returns a map with the latest metrics of matching name

	fmt.Printf(" Peer: %s\n", (obj.Peer).String())
	fmt.Printf(" Peer: %s\n", obj.Peer.Pretty())

	Usage: "Get latest metrics of matching name for the current cluster peers.",
	Usage: "List latest metrics logged by this peer",

Issue #449 API endpoint for Peer Monitor metrics #572

Issue #449 API endpoint for Peer Monitor metrics #572

Conversation

kishansagathiya commented Oct 7, 2018 • edited

coveralls commented Oct 7, 2018 • edited

kishansagathiya commented Oct 8, 2018

hsanjuan 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

hsanjuan left a comment

Choose a reason for hiding this comment

kishansagathiya commented Oct 23, 2018

hsanjuan commented Oct 23, 2018

kishansagathiya commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya commented Oct 7, 2018 •

edited

coveralls commented Oct 7, 2018 •

edited