Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics: pretty ugly d3 plotting #40

Closed
wants to merge 2 commits into from
Closed

Conversation

cryptix
Copy link

@cryptix cryptix commented Apr 30, 2015

bare with me, my d3 and react isn't very good.. ipfs/kubo#1163 adds an API endpoint on port 5001 that exposes metrics about the memory consumption of the daemon (memstats) and custom metrics about the datastores. (here is a sample of the JSON)

I like to visualize those, after some googling I stubmled over some handwoven d3 react magic and finally
react-d3.

Currently it looks like this:

stats

but after some time the plots stop to update and I get this in the console:

Uncaught Error: Invariant Violation: setState(...): Cannot update during an existing state transition (such as within `render`). Render methods should be a pure function of props and state.

Like I said, my reactjs knowledge isn't very good (this might be my first public commit) and I wanted some feedback (and maybe a better plotting lib suggestion?) on the general approach before I sink more time into this.

Obvious TODOS:

  • time on the x axis
  • formatting the Y axis properly
  • hiding data lines by clicking the legends?
  • the custom metrics also have histogram data in percentiles. Might want another chart type for that.

ps: I needed to add /debug/ to ipfs-node-server-static so that the requests are proxied properly.

@jbenet jbenet added the status/deferred Conscious decision to pause or backlog label Apr 30, 2015
@travisperson
Copy link
Member

Awesome!

So a few things on ReactJS (see https://facebook.github.io/react/docs/component-specs.html).

Looks like some of this was covered in any resources you may of found, or maybe you found the page above. I personally am not amazing at ReactJS, but I would say we would want to set the interval for getData(); inside of componentDidMount, as it only runs on the client. Not a big deal though, I don't think it will make a difference in this case.

Right off the bat for the error you are getting I would of assume it was because you switched views, causing your time to be cleared, but the http request would of still been in-flight. But this doesn't appear to be the case for you as it just happening as you watch the plots?

@jbenet
Copy link
Member

jbenet commented May 5, 2015

I'd like to have this in soon, cause i want to inspect my nodes. what's missing here? @cryptix @travisperson

@travisperson
Copy link
Member

I haven't pulled this one down yet, but I'll take a closer look tonight. Currently in the middle of exams (school).

@cryptix
Copy link
Author

cryptix commented May 5, 2015

thanks Travis! I'm really fishing in the dark here. :)

On 05.05.2015 18:16, Travis Person wrote:

I haven't pulled this one down yet, but I'll take a closer look tonight. Currently in the middle of exams (school).


Reply to this email directly or view it on GitHub:
#40 (comment)

@whyrusleeping whyrusleeping mentioned this pull request May 11, 2015
30 tasks
@jbenet
Copy link
Member

jbenet commented May 16, 2015

I'd like to be able to use this -- @travisperson ?

@travisperson
Copy link
Member

@cryptix Where are you exposing debug/vars for development? When I pull down the PR and run it, I get errors for not being able to request debug/vars. I can curl it http://localhost:5001/debug/vars, but it doesn't appear to be proxying.

I'm assuming you went in and modified the ipfs-node-server-static code, to remove the check for the api value in the first part?

@travisperson
Copy link
Member

Just a note: I'm not sure if this is 100% why this error occurs, but this is what I've found from the stack trace. The error occurs on the interval, and not when you hover over a circle. Also it appears to only occur after a period of time.


Pretty sure this is an issue with react-d3. I haven't found any references so maybe we are just using it wrong, but this is what I have come up with as to why there is that error.

Inside of the DataSeries the react-d3 lib uses immstruct which is an observable.

        // Create an immstruct reference for the circle id
        // and set it to 'inactive'
        props.structure.cursor('voronoi').set(id, 'inactive');

        // Having set the Voronoi circle id cursor to 'inactive'
        // We now pass on the Voronoi circle id reference to the
        // circle component, where it will be observed and dereferenced
        var voronoiRef = props.structure.reference(['voronoi', id]);

        return (
          <Circle
            voronoiRef={voronoiRef}
            voronoiSeriesRef={voronoiSeriesRef}
            structure={props.structure}
            cx={cx}

https://github.com/esbullington/react-d3/blob/master/src/linechart/DataSeries.jsx#L106

The circle component is listening for these changes, and when it happens it calls _restoreCircle, which runs a setState

componentDidMount:function() {
    var props = this.props;
    // The circle reference is observed when both it is set to
    // active, and to inactive, so we have to check which one
    var unobserve = props.voronoiRef.observe(function()  {
      var circleStatus = props.voronoiRef.cursor().deref();
      var seriesName = props.id.split('-')[0];
      if (circleStatus === 'active') {
        this._animateCircle(props.id);
        props.structure.cursor('voronoiSeries').cursor(seriesName).update(function(){return 'active';});
      } else if (circleStatus === 'inactive') {
        this._restoreCircle(props.id);
        props.structure.cursor('voronoiSeries').cursor(seriesName).update(function(){return 'inactive';});
      }
    }.bind(this));
  },

https://github.com/esbullington/react-d3/blob/master/src/linechart/Circle.jsx#L43

  _restoreCircle(id) {
    this.setState({ 
      circleRadius: this.props.r
    });
  }

https://github.com/esbullington/react-d3/blob/master/src/linechart/Circle.jsx#L73

Since the props.structure.cursor('voronoi').set(id, 'inactive'); is happening inside of a render method, we get this error being thrown back at us.

@cryptix
Copy link
Author

cryptix commented May 16, 2015

@travisperson thanks for looking at this!

I'm assuming you went in and modified the ipfs-node-server-static code, to remove the check for the api value in the first part?

Yup, I patched it here to allow /debug as well. I should have posted a PR for this as well.

@krl
Copy link
Contributor

krl commented May 18, 2015

Ok so I just looked at this, and have some questions/opinions

If we want to export this debug information, i really think it should go through the ipfs api, like everything else, not through custom http requests.

It does not make a whole lot of sense to add implementation specific stuff to the api though, so maybe we could have a /metrics url that would just return grouped key-value pairs of whatever your implementation supports?

@jbenet, @cryptix, @tperson any thoughts?

@krl
Copy link
Contributor

krl commented May 18, 2015

Another thing, if we're going this way, we should probably also do the saving of the values in the backend, so that you don't have to keep the cache of older values in the browser. We should probably also support range queries, like 'last 5 minutes, last 30 minutes' etc.

@cryptix
Copy link
Author

cryptix commented May 18, 2015

@krl: I tried to convey this with the meme picture in the initial post. I don't have any strong feelings about any of this. I just wanted to get this out there to show that we have this data now.

re format: I think this is pretty specific to the go implementation and I'm not sure adding it to the IPFS API and specifying it for ipfs in general makes much sense.

IMHO we should be fine with documenting the json values we expose through expvar and what they mean.

re storing metrics: I don't know.. running every node with a full fledged time series DB sounds excessive. I'd be more in favor of either supporting influxdb or Prometheus backends, depending on wether we want to support a pull or a push model and leaving this as a pure feed/stream.

@cryptix
Copy link
Author

cryptix commented May 20, 2015

Stubmled over this blog post yesterday. Maybe someone with a higher webui-belt can vet this approach?

@dignifiedquire
Copy link
Member

@cryptix is this something you are still looking into finishing, or should we close this for now?

@cryptix
Copy link
Author

cryptix commented Nov 19, 2015

I'm fine with closing. I had zero time to get my teeth into frontend recently... :-/

I hope somebody does this sometime for network throughout / ram useage / etc..

The data is already there with the Prometheus endpoints.

On 19.11.2015, at 18:29, Friedel Ziegelmayer notifications@github.com wrote:

@cryptix is this something you are still looking into finishing, or should we close this for now?


Reply to this email directly or view it on GitHub.

@jbenet
Copy link
Member

jbenet commented Nov 19, 2015

It would be really, really nice to have it built into the webui. once the webui modularizes we can have an explosion of little apps to try.

we should come up with a good permissions model for them-- i.e. grant permissions to an app to a strict subset of the API. (say RO, or on subset of commands, etc).

since our API is a command tree, we could create policies that effectively do this:

deny *
allow ipfs/object/get
allow ipfs/cat
allow ipfs/bitswap/stat
allow ipfs/logs?filter=bitswap

This is now moved to https://github.com/ipfs/api/issues/2

@dignifiedquire
Copy link
Member

Thanks @jbenet closing this here. I fully agree that this would be nice and will be adding it to the list of nice visualizations/apps to have :)

@jbenet jbenet removed the status/deferred Conscious decision to pause or backlog label Nov 19, 2015
RichardLitt added a commit to RichardLitt/webui that referenced this pull request Dec 27, 2015
Converted most of the BS elements to ReactBootstrap, as per ipfs#40. I did not do all of them - I am not sure how Button would react, for instance, and I want to make sure I do it right before I mess something up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants