Remove pod-level metrics from web and CLI #304

hawkw · 2018-02-08T19:53:54Z

This PR updates the web UI to remove the pod detail page, and to remove the links to that page from pod names in metrics tables. It also removes the pods option from conduit stat, and the sourcePod and targetPod fields from the controller API proto's MetricMetadata message.

I've updated the conduit stat tests to reflect these changes, and manually verified the web UI changes.

Closes #261

Signed-off-by: Eliza Weisman <[email protected]>

adleong

I think there is more pod-level data displayed on the deployment detail page: pod-summary, pod-distribution-chart, etc.

adleong · 2018-02-08T20:32:11Z

web/app/js/components/TabbedMetricsTable.jsx

@@ -11,10 +11,10 @@ import { Table } from 'antd';
 const resourceInfo = {
  "upstream_deployment": { title: "deployment", url: "/deployment?deploy=" },
  "downstream_deployment": { title: "deployment", url: "/deployment?deploy=" },
-  "upstream_pod": { title: "upstream pod", url: "/pod?pod=" },
-  "downstream_pod": { title: "downstream pod", url: "/pod?pod=" },
+  "upstream_pod": { title: "upstream pod", url: null },


should these lines be removed entirely?

@adleong removing those lines entirely breaks the deployment detail page --- I wasn't sure if we still wanted to render the list of pods in the deployment. If we don't, I can remove that as well.

you should be able to remove these lines, nothing's using them now, afaict

whoops, totally forgot about that --- removed in a76ea00

Signed-off-by: Eliza Weisman <[email protected]>

rmars

Nice!
There's a lint warning on this branch that should be cleaned up.

There's some more references to pod fetching in ApiHelpers.jsx that could be removed (in urlsForResource)

rmars · 2018-02-08T21:43:32Z

web/app/js/components/DeploymentDetail.jsx

@@ -51,7 +50,7 @@ export default class DeploymentDetail extends React.Component {
      pollingInterval: 10000,
      deploy: deployment,
      metrics: [],
-      pods: [],
+      // pods: [],


I would just delete this rather than commenting it out.
we're now at the point where we're removing so much that I think it'll just be clutter if we continue to comment things out.

Yeah, I meant to comment it out while testing the change, and then delete everything that was commented out, but I think I missed that one, thanks.

rmars · 2018-02-08T21:53:45Z

web/app/js/components/DeploymentDetail.jsx

@@ -160,55 +148,22 @@ export default class DeploymentDetail extends React.Component {
  }

  renderMidsection() {


TIOLI the Deployment Details section is quite awkward on a row by itself, and it was slated to be removed anyway. I'd just remove all of renderMidsection() (or I will, in a follow up branch; there's more things that we're taking out too).

rmars · 2018-02-08T21:56:04Z

web/app/js/components/DeploymentDetail.jsx

@@ -12,7 +11,7 @@ import { rowGutter } from './util/Utils.js';
 import TabbedMetricsTable from './TabbedMetricsTable.jsx';
 import UpstreamDownstream from './UpstreamDownstream.jsx';
 import { Col, Row } from 'antd';
-import { emptyMetric, getPodsByDeployment, processRollupMetrics, processTimeseriesMetrics } from './util/MetricUtils.js';
+import { emptyMetric, processRollupMetrics, processTimeseriesMetrics } from './util/MetricUtils.js';


emptyMetric should go too - there are lint warnings on this branch

/Users/mars/workspace/go/src/github.com/runconduit/conduit/web/app/js/components/DeploymentDetail.jsx 14:10 warning 'emptyMetric' is defined but never used no-unused-vars ✖ 1 problem (0 errors, 1 warning)

rmars · 2018-02-08T22:01:07Z

web/app/js/components/DeploymentDetail.jsx

-    this.serverPromise = Promise.all([deployFetch, podRollupFetch, upstreamFetch, downstreamFetch, podListFetch, pathsFetch])
-      .then(([deployMetrics, podRollup, upstreamRollup, downstreamRollup, podList, paths]) => {
+    this.serverPromise = Promise.all([deployFetch, upstreamFetch, downstreamFetch, pathsFetch])
+      .then(([deployMetrics,upstreamRollup, downstreamRollup, paths]) => {


nit: space after comma in deployMetrics,upstreamRollup

hawkw · 2018-02-08T22:05:53Z

BTW, after testing this branch against a cluster running the latest Conduit nightly, I noticed some pages failing to load. I think that's due to the protobuf changes, and I'm building a dev version of Conduit to test against in order to make sure that's the case

siggy

after going through the full setup in BUILD.md, i noticed the Deployment Detail pages indicate the deployments are UNADDED, even though they are:

hawkw · 2018-02-08T22:21:46Z

@siggy yeah, I just noticed that as well. Am currently trying to figure out what I broke.

Signed-off-by: Eliza Weisman <[email protected]>

hawkw · 2018-02-09T00:33:46Z

Okay folks, metrics work again and the inadvertent UNADDED badge has been removed:

I think this is review-able again!

Signed-off-by: Eliza Weisman <[email protected]>

adleong · 2018-02-09T01:06:36Z

controller/telemetry/server.go

-	sourceName, sourceDeployment := s.getNameAndDeployment(requestScope.Ctx.SourceIp)
-	targetName, targetDeployment := s.getNameAndDeployment(requestScope.Ctx.TargetAddr.Ip)
+	var sourceDeployment string
+	sourceDeploymentPtr, _ := s.getDeployment(requestScope.Ctx.SourceIp)


if we always discard the error then why have getDeployment return an error at all?

due to a poorly-conceived attempt at forwards-compatibility. simplified in 62f7927

adleong · 2018-02-09T01:07:06Z

controller/telemetry/server.go

-	targetName, targetDeployment := s.getNameAndDeployment(requestScope.Ctx.TargetAddr.Ip)
+	var sourceDeployment string
+	sourceDeploymentPtr, _ := s.getDeployment(requestScope.Ctx.SourceIp)
+	if sourceDeploymentPtr == nil {


If we always replace nils with empty strings, why not have getDeployments just return empty strings in the first place?

adleong · 2018-02-09T01:09:06Z

proto/public/api.proto

@@ -66,9 +66,8 @@ message MetricSeries {
 }

 message MetricMetadata {
-  string targetPod = 1;
+  reserved 1, 3;


Our histogram change probably already broke API compatibility so 0.2 -> 0.3 is going to be a breaking change. Therefore, I don't think we need to reserve fields 1 and 3. 🍳

Signed-off-by: Eliza Weisman <[email protected]>

adleong · 2018-02-09T01:23:20Z

proto/public/api.proto

@@ -66,9 +66,7 @@ message MetricSeries {
 }

 message MetricMetadata {
-  string targetPod = 1;


Can condense the existing fields to make use of the newly available field numbers.

I didn't do this because the gRPC docs said it was considered harmful. But if we're not concerned about compatibility, can change.

Signed-off-by: Eliza Weisman <[email protected]>

adleong

⭐ Eggs-cellent! 🥚

rmars

⭐️ 🚳 📻

* 80b4ec5 (tag: v0.1.13) Bump version to v0.1.13 (linkerd#324) * 6b23542 Add client support for server push (linkerd#314) * 6d8554a Reassign capacity from reset streams. (linkerd#320) * b116605 Check whether the send side is not idle, not the recv side (linkerd#313) * a4ed615 Check minimal versions (linkerd#322) * ea8b8ac Avoid prematurely unlinking streams in `send_reset`, in some cases. (linkerd#319) * 9bbbe7e Disable length_delimited deprecation warning. (linkerd#321) * 00ca534 Update examples to use new Tokio (linkerd#316) * 12e0d26 Added functions to access io::Error in h2::Error (linkerd#311) * 586106a Fix push promise frame parsing (linkerd#309) * 2b960b8 Add Reset::INTERNAL_ERROR helper to test support (linkerd#308) * d464c6b set deny(warnings) only when cfg(test) (linkerd#307) * b0db515 fix some autolinks that weren't resolving in docs (linkerd#305) * 66a5d11 Shutdown the stream along with connection (linkerd#304)

hawkw added 5 commits February 8, 2018 11:12

Remove pod detail page

41cdd88

Signed-off-by: Eliza Weisman <[email protected]>

Remove link to pods in TabbedMetricsTable

c8243df

Signed-off-by: Eliza Weisman <[email protected]>

Remove conduit stat pods command

f4d7218

Signed-off-by: Eliza Weisman <[email protected]>

Remove TargetPod/SourcePod fields from web api handlers

60a9948

Signed-off-by: Eliza Weisman <[email protected]>

Remove sourcePod field from MetricMetadata

2c5b437

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested review from pcalcado, rmars and adleong February 8, 2018 19:53

hawkw added the review/ready Issue has a reviewable PR label Feb 8, 2018

pcalcado added area/controller area/cli labels Feb 8, 2018

adleong reviewed Feb 8, 2018

View reviewed changes

Remove entire pods list from deployment detail page

c9a9db4

Signed-off-by: Eliza Weisman <[email protected]>

siggy assigned hawkw Feb 8, 2018

rmars reviewed Feb 8, 2018

View reviewed changes

siggy reviewed Feb 8, 2018

View reviewed changes

hawkw added 3 commits February 8, 2018 16:15

debreak public-api Stat requests

bca9563

Signed-off-by: Eliza Weisman <[email protected]>

Fix deployments in the mesh being marked as "unadded"

cb9d647

Remove renderMidsection() as per review from @rmars

b6b51b8

Signed-off-by: Eliza Weisman <[email protected]>

hawkw added 2 commits February 8, 2018 16:39

rm unused

8542e23

Signed-off-by: Eliza Weisman <[email protected]>

review feedback

a76ea00

Signed-off-by: Eliza Weisman <[email protected]>

adleong reviewed Feb 9, 2018

View reviewed changes

Review feedback

62f7927

Signed-off-by: Eliza Weisman <[email protected]>

adleong reviewed Feb 9, 2018

View reviewed changes

Review feedback

8261211

Signed-off-by: Eliza Weisman <[email protected]>

adleong approved these changes Feb 9, 2018

View reviewed changes

rmars approved these changes Feb 9, 2018

View reviewed changes

hawkw merged commit 2015d99 into master Feb 9, 2018

hawkw removed the review/ready Issue has a reviewable PR label Feb 9, 2018

This was referenced Feb 9, 2018

Remove pod-level metrics from telemetry pipeline #262

Closed

What to do with conduit stat options after removing metric aggregation types? #316

Closed

Remove per-pod metrics aggregation types #318

Closed

klingerf mentioned this pull request Feb 14, 2018

Remove unused web routes and helper #356

Merged

olix0r deleted the eliza/rm-pod-detail branch April 30, 2018 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pod-level metrics from web and CLI #304

Remove pod-level metrics from web and CLI #304

hawkw commented Feb 8, 2018

adleong left a comment

adleong Feb 8, 2018

hawkw Feb 8, 2018

rmars Feb 9, 2018 •

edited

Loading

hawkw Feb 9, 2018

rmars left a comment

rmars Feb 8, 2018

hawkw Feb 8, 2018

rmars Feb 8, 2018

rmars Feb 8, 2018

rmars Feb 8, 2018

hawkw commented Feb 8, 2018

siggy left a comment

hawkw commented Feb 8, 2018

hawkw commented Feb 9, 2018

adleong Feb 9, 2018

hawkw Feb 9, 2018

adleong Feb 9, 2018

adleong Feb 9, 2018

adleong Feb 9, 2018

hawkw Feb 9, 2018

adleong left a comment

rmars left a comment

		@@ -160,55 +148,22 @@ export default class DeploymentDetail extends React.Component {
		}

		renderMidsection() {

Remove pod-level metrics from web and CLI #304

Remove pod-level metrics from web and CLI #304

Conversation

hawkw commented Feb 8, 2018

adleong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmars Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmars 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

hawkw commented Feb 8, 2018

siggy left a comment

Choose a reason for hiding this comment

hawkw commented Feb 8, 2018

hawkw commented Feb 9, 2018

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

adleong left a comment

Choose a reason for hiding this comment

rmars left a comment

Choose a reason for hiding this comment

rmars Feb 9, 2018 •

edited

Loading