-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Record Datastore metrics for flatfs and leveldb #1163
Conversation
LGTM will merge when tests finish |
Okay I see the test failures, will do something about that.. |
I used the sequence counter in the name because then real world use leaves the metrics names clean, and only tests and weird situations suffer. Normal runs have just one FSRepo, so normal runs get It's in the FSRepo because that's where the leveldb and flatfs are opened, where mount.New is, and all knowledge of the shape of the datastore; that lets me put metrics at the right locations, yet only do the work once and not in all of leveldb, flatfs, etc. |
The travis failures above are:
Those seem unrelated. |
Changes look 💎 clean to me. Also tested it locally and found something juicy already... :) |
~~This is not reusable outside of ipfs. it breaks abstraction. I can't use multiple metrics datastores in places. ~~ nevermind, it is. the prefix works. Also, it is not correct that "Normal runs have just one FSRepo". I use tools that embed multiple ipfs nodes in one process.
then again, it may be that prefix is complex to use (this here is a good example), and it is the right thing to fix it over in https://github.com/jbenet/go-datastore/tree/master/measure -- to make sure that every metrics datastore does not have the ability to clash with another. (i.e. either remove the prefix, or use an atomic counter there). this reminds me of linux's insistence that we use XXX with tmpfiles.
Then an atomic counter may be the right thing. I really want to not rely on that package lock. ideally we can get rid of it now. |
updated comment o/ |
By "normal" I mean purely what my experience has been so far: I am not happy with the numerical uniqueness suffix thing. At all. But I couldn't come up with anything better, and it makes the tests pass. As far as I can see, the only unique ID FSRepo currently has is the pathname of the config file. I really don't want to put that in the metrics identifier. The only nicer solution I can see right now is somehow giving names of some sort to every FSRepo instance you open. Then your tools that embed multiple ipfs nodes in one process can name then in an understandable way (or fall back to numbers or hashes, with the hope that there is some meaning to the user there). I'm really open to proposals on how you want this done. My constraints are:
How do we proceed from here? |
merged ipfs/go-datastore#20 |
(looks like this needs rebasing too?) |
And two commits on top, re-vendoring ipfs/go-datastore#20 and calling the Close. I have them waiting, will get to it later today. |
Alright this seems good to go, the Travis error is https://travis-ci.org/ipfs/go-ipfs/jobs/60868636#L2773-L2803 with t0101-iptb-name.sh saying "Error: routing: not found" and so on -- that seems unrelated. |
return mux, nil | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something we want to enable all the time? or does it not doing anything unless you access it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're just handlers for /debug/vars
and /debug/pprof
, nothing happens unless somebody sends a HTTP request for those. I'm not sure I understand the questions.
This LGTM. |
Record Datastore metrics for flatfs and leveldb
Inspirational command lines: