Add ability to change the time window for metrics fetching throughout the app by rmars · Pull Request #237 · linkerd/linkerd2

rmars · 2018-01-31T01:24:25Z

Changes

Consolidate the usage of pathPrefix and metricsWindow into ApiHelpers.jsx.
- This allows us to stop passing around metricsWindow and pathPrefix in each component in the app.
- This also allows us to set the metricsWindow and have this setting stay as we navigate around the app
Wrap all app Links in a ConduitLink that includes the pathPrefix, so that we don't need pathPrefix everywhere
Add tests for ApiHelpers

Current issues

Until the telemetry service is refactored, the 1h time window is probably never going to return metrics. Also, 10s is probably not going to make a lot of sense as an option you want to view.
Currently, it's not super evident that your time window change has been received when you click the buttons. We could clear the metrics for the page and show a spinner immediately, which would give the user feedback. I've tried this in af320b0

Future work enabled by this branch

setting the pollingInterval based on the metricsWindow (Set pollingInterval based on metricsWindow #241)
wrapping the api calls in a cancelable promise, which will allow us to stop setting state after component unmount (Add the ability to cancel metrics fetches #242, React setState warnings on component navigation #157)

siggy

looks good!

i see what you mean about the responsive of switching time windows. is it possible to initiate a metrics update immediately when the button is clicked? if not, some feedback that stuff is happening is helpful. also, if this makes sense to do in a subsequent branch, that's cool too.

siggy · 2018-01-31T18:44:40Z

web/app/js/components/util/ApiHelpers.jsx

+    "1m": true,
+    "10m": true,
+    "1h": true
+  };


agree with your comment that 10s may not make sense, particularly if our collection interval is never less than that.

assuming smallest collection interval is 10s, a window of 1m still only gives 6 data points. assuming our graphs can always handle >=30 data points, perhaps our smallest window should be 5m?

also assuming our telemetry eventually scales, i think a max window of 24h or greater could be useful.

Yeah, that seems reasonable. From api.proto we currently have:

enum TimeWindow { TEN_SEC = 0; ONE_MIN = 1; TEN_MIN = 2; ONE_HOUR = 3; }

Happy to update this pending telemetry improvements. Will make a ticket and revisit in the future.

siggy · 2018-01-31T18:54:49Z

web/app/js/components/TabbedMetricsTable.jsx

    this.setState({ pendingRequests: true });

-    this.api.fetch(this.state.metricsUrl.ts)
+    this.api.fetchMetrics(this.state.metricsUrl.ts)


a few lines below this, 10 MINUTE HISTORY is hard-coded.

omg, I thought I got all of those. thanks for catching!

siggy · 2018-01-31T19:02:41Z

web/app/js/components/util/ApiHelpers.jsx


+  const fetchMetrics = (path = metricsPath()) => {
+    if (path.indexOf("window") === -1) {
+      path = `${path}&window=${getMetricsWindow()}`;


will this generate URLs like /foo/bar&window=baz ? should we ensure we're inserting a ? between the path and query string?

rmars · 2018-01-31T20:45:24Z

is it possible to initiate a metrics update immediately when the button is clicked

Yeah definitely, I think we should. It might be nice to combine this with the cancelable promises branch [ticket forthcoming]. I didn't want to include all those changes in this branch, but I'll look into placing a lightweight spinner in the meantime.

rmars · 2018-01-31T21:40:33Z

Okay, I've opted for just clearing the current page's metrics (and forcing the page-wide spinner) when the time window is changed, which is probably the most informative thing for now. lmk what you think.

- Add buttons on metrics pages to control window of metrics requests - Consolidate metricsWindow usage (stop passing it around) - Add a ConduitLink component so we can stop passing around pathPrefix - Add tests for ApiHelpers Signed-off-by: Risha Mars <[email protected]>

Signed-off-by: Risha Mars <[email protected]>

adleong · 2018-02-01T22:38:03Z

There's a lot we can do with respect to setting the polling interval based on the window size, managing state transitions when changing window sizes, figuring out the optimal number of datapoints per graph, etc etc etc but.... I wonder if this will be wasted effort if we want to eventually offload this kind of logic onto Grafana.

At the moment I think 10m is the only window size we can display well so maybe we just want to stick to that until we know what the plan will be going forward.

rmars · 2018-02-01T23:29:06Z

Yeah that's a good point, it's not very neccessary right now to go too deep into the pollingInterval / graph things. The buttons are very easy to hide (and no need to worry about transitions if there aren't any buttons to be clicked).

The underlying API fetching refactor where we stop passing around metricsWindow and pathPrefix is still very helpful from a cleanup perspective in my mind.

…clicked" This reverts commit 5cb5457.

siggy

thanks! one comment and then 🚢

siggy · 2018-02-02T19:15:09Z

web/app/js/components/PageHeader.jsx

            {!this.props.subHeader ? null : <h1>{this.props.subHeader}</h1>}
          </Col>
-          { this.props.hideButtons ? null :
+          {/* { this.props.hideButtons ? null :


mind leaving a comment about disabling this?

yeah good idea!

klingerf

This is a much-needed refactor! I had some nit picky comments but overall this is a huge step in the right direction.

klingerf · 2018-02-02T21:54:36Z

web/app/test/testHelpers.jsx

+const componentDefaultProps = { api: ApiHelpers("") };
+
 export function routerWrap(Component, extraProps={}, route="/", currentLoc="/") {
+  const createElement = (Component, props) => <Component {...(_.merge({}, componentDefaultProps, props, extraProps))} />;


Just curious, isn't this Component argument shadowing the one that's passed into routerWrap? How would you feel about renaming it for clarity?

ah yeah, good point. will rename.

klingerf · 2018-02-02T21:58:18Z

web/app/test/testHelpers.jsx

-      <Route path={route} component={Component} />
+    <Router history={createMemoryHistory(currentLoc)} createElement={createElement}>
+      <Route
+        path={route} render={props => <Component {...(_.merge({}, componentDefaultProps, props, extraProps))} />} />


Can you use the createElement function here? Something like:

path={route} render={props => createElement(Component, props)} />

klingerf · 2018-02-02T22:08:28Z

web/app/js/components/util/ApiHelpers.jsx

    return fetch(path).then(handleFetchErr).then(r => r.json());
  };

+  const fetchMetrics = (path = metricsPath()) => {


TIOLI I find it a bit confusing that the path param here has a default value. In all but one use case, you're supplying the path when calling this method. I'd recommend just making it required. Then you could get rid of the metricsPath const, and it would be more clear what path is being used by all of the callers.

klingerf · 2018-02-02T22:19:36Z

web/app/test/ApiHelpersTest.jsx

+      api.fetchMetrics('/my/path');
+
+      expect(fetchStub.calledOnce).to.be.true;
+      expect(fetchStub.args[0][0]).to.equal('/my/path?window=10m');


I don't think this test description matches the test, since you're not re-initializing api with a path prefix. Maybe it would be better to do something like:

it('adds pathPrefix and metricsWindow to a metrics request', () => { api = ApiHelpers('/the/path/prefix'); api.fetchMetrics('/my/path'); expect(fetchStub.calledOnce).to.be.true; expect(fetchStub.args[0][0]).to.equal('/the/path/prefix/my/path?window=10m'); });

oh yesh, whoooops, thank you for catching!

klingerf · 2018-02-02T22:20:37Z

web/app/test/ApiHelpersTest.jsx

+      expect(fetchStub.args[0][0]).to.equal('/metrics?foo=3&bar="me"&window=10m');
+    });
+
+    it('does not add another &window= there is already a window param', () => {


Typo in the description here:

does not add another &window= if there is already a window param

klingerf · 2018-02-02T22:25:37Z

web/app/js/components/Sidebar.jsx

            </Menu.Item>
            <Menu.Item className="sidebar-menu-item" key="/docs">
-              <Link to="https://conduit.io/docs/" target="_blank">Documentation</Link>
+              <ConduitLink to="https://conduit.io/docs/" absolute="true" target="_blank" name="Documentation" />


I don't think you need the target="_blank" prop here. That's set automatically by the absolute="true" prop.

klingerf · 2018-02-02T22:33:32Z

web/app/js/components/Sidebar.jsx

        <div className="list-container">
          <div className="sidebar-headers">
-            <Link to={`${this.props.pathPrefix}/servicemesh`}><img src={logo} /></Link>
+            <ConduitLink to="/servicemesh" name={<img src={logo} />} />


🤔 name doesn't seem like quite the right name for that prop in this context, but I can't think of a better one. I guess you could convert ConduitLink to not be a self-closing component, and instead reference props.children inside the component, to get the element to link. Something like:

<ConduitLink to="/servicemesh"><img src={logo} /></ConduitLink>

Yeah! I like that!

klingerf · 2018-02-02T22:46:19Z

web/app/js/components/DeploymentSummary.jsx

        <div className="border-container-content">
          <div className="summary-title">{this.title()}</div>
-          <div className="summary-info">Last 10 minutes RPS</div>
+          <div className="summary-info">RPS (last {this.props.api.getMetricsWindow()})</div>


TIOLI -- you could add a separate method that returns a displayable version of the metrics window, and then use that whenever rendering the metrics window to text. Something like:

> api.getMetricsWindow(); "10m" > api.getMetricsWindowDisplay(); "10 minutes"

Or... something like that. But I'm also ok with the shorthand, if we think it's clear enough for users.

ooh yeah, that's a good idea!

klingerf · 2018-02-02T23:11:00Z

web/app/js/components/PageHeader.jsx

+  renderMetricWindowButtons() {
+    // don't use time window changing until the results of Telemetry Scalability are in
+    // https://github.com/runconduit/conduit/milestone/4
+    if (this.props.hideButtons) {


I was initially a bit confused by this comment, since I thought it applied to line 23 below (the this.props.hideButtons check). But it really applies to this method as a whole. In that case I might consider moving the comment to where the method call is commented out, on line 50 of this file.

what do you think about moving the comment outside (just above) the method instead?

Cool, yeah, that works for me too.

klingerf · 2018-02-02T23:16:16Z

web/app/js/components/DeploymentDetail.jsx

+            <PageHeader
+              subHeaderTitle="Deployment detail"
+              subHeader={this.renderDeploymentTitle()}
+              subMessage={incompleteMeshMessage(this.state.deploy)}


I think this needs to be:

subMessage={!this.state.added ? incompleteMeshMessage(this.state.deploy) : null}

incompleteMeshMessage never returns null, but the render method in the PageHeader component has:

{!this.props.subMessage ? null : <div>{this.props.subMessage}</div>}

That results in us displaying the incomplete mesh message on every deployment page, even for those that have been added to the mesh. For instance:

whoops, yes, that check got lost in the porting. nice catch!

…Message

klingerf

⭐️ Looks great, thanks for making those updates!

commit 073a1beb4a7cd709c6b1eaa56a319c1829a94d11 Author: Sean McArthur <[email protected]> Date: Mon Apr 29 17:54:01 2019 -0700 tap: remove need to clone Services (#238) This refactors the tap system to not require intermediary channels to register matches and taps when a request comes through. The Dispatcher that used to exist in order to prevent tapping more requests than the limit asked for has been removed. In its place is a shared atomic counter to keep the count under the limit. The resulting behavior should be the same. There should be improved performance as tap registration doesn't need go through a second channel, and requests don't need to be delayed waiting for the dispatcher to be able to process its queue. Signed-off-by: Sean McArthur <[email protected]> commit 7a3be8c8737188e5debbc465f9a33da0d79b8b80 Author: Zahari Dichev <[email protected]> Date: Wed May 1 01:57:01 2019 +0300 Replace fixed reconnect backoff with exponential one (#237) When reconnecting to a destination, use an exponential, jittered backoff strategy. Signed-off-by: Zahari Dichev <[email protected]> commit 32b813aad4fe2fcf0252e8c2215d6835101d2337 Author: Oliver Gould <[email protected]> Date: Tue Apr 30 15:58:20 2019 -0700 Support endpoint weights (#230) This change modifies the proxy to honor weights provided by the destination service. When the destination service replies with a weight, this value is divided by 10,000 to produce a weight on [0.0, ~400000.0]. This weight is used by load the load balancer to modify load interpretation and therefore request distribution. A weight of 0.0 will cause the endpoint's load to be effectively infinite so that requests will only be sent to the endpoint when no other endpoints exists or when the other endpoints that were considered had 0-weights. commit 501802671a346250b6dbaae73f29d9be7a4c2086 Author: Sean McArthur <[email protected]> Date: Wed May 1 13:42:38 2019 -0700 Remove buffers from endpoint stacks (#239) Due to the `http::settings::router`, a `buffer` was needed in each endpoint stack. This meant that the service was always ready, even if the client were falling over (and reconnecting). In turn, this meant that the balancer would pick one of these endpoint stacks, because it was always ready! This change includes a test of a failing endpoint, that the balancer no longer assumes it is ready, and has the following functional changes: - Removed `http::settings::router`, instead the client HTTP settings are detected as part of the `DstAddr`. This means that each balancer only has endpoints with the same HTTP settings. - Removed `buffer` layer from inside the endpoint stacks. Signed-off-by: Sean McArthur <[email protected]>

rmars added the area/web label Jan 31, 2018

rmars self-assigned this Jan 31, 2018

rmars added the review/ready Issue has a reviewable PR label Jan 31, 2018

rmars force-pushed the rmars/set-window branch 2 times, most recently from f7267bd to a2904ac Compare January 31, 2018 01:50

siggy reviewed Jan 31, 2018

View reviewed changes

rmars mentioned this pull request Jan 31, 2018

Set pollingInterval based on metricsWindow #241

Closed

rmars added 3 commits February 1, 2018 09:53

Fix adding of ? in url, add tests, Don't hardcode 10 minutes

8d66354

Signed-off-by: Risha Mars <[email protected]>

Clear page data and force spinner if time window buttons are clicked

5cb5457

Signed-off-by: Risha Mars <[email protected]>

rmars requested a review from klingerf February 1, 2018 22:18

rmars force-pushed the rmars/set-window branch from af320b0 to 5cb5457 Compare February 1, 2018 22:19

rmars added 2 commits February 1, 2018 15:33

Revert "Clear page data and force spinner if time window buttons are …

197a600

…clicked" This reverts commit 5cb5457.

Hide the time window buttons; fix bug in absolute links

79d2157

siggy approved these changes Feb 2, 2018

View reviewed changes

Add a note explaining why metricWindow buttons are disabled

890b5ea

klingerf reviewed Feb 2, 2018

View reviewed changes

rmars added 3 commits February 2, 2018 16:53

Add a getMetricsWindowDisplayText method, fix check on incompleteMesh…

f107568

…Message

Convert ConduitLink in to a component that wraps another

0d534f9

Miscellaneous test/naming tweaks

45aa1fc

klingerf approved these changes Feb 5, 2018

View reviewed changes

rmars merged commit 9887f10 into master Feb 5, 2018

rmars removed the review/ready Issue has a reviewable PR label Feb 5, 2018

rmars deleted the rmars/set-window branch February 5, 2018 18:56

Conversation

rmars commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Current issues

Future work enabled by this branch

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmars commented Jan 31, 2018

Uh oh!

rmars commented Jan 31, 2018

Uh oh!

adleong commented Feb 1, 2018

Uh oh!

rmars commented Feb 1, 2018

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

rmars commented Jan 31, 2018 •

edited

Loading

klingerf Feb 2, 2018 •

edited

Loading