stubI want to land https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/18198 today so I can turn on the new logging on edge on the weekend.07:00
stubrockstar: ^^07:01
stubjtv1: And I even used real ISO8601 date formats without a / to be found!07:03
jtv1stub: I'm so proud07:03
jtv1But I suppose this is your way of asking for a review.07:04
stubI'm trying subtlety before resorting to abuse and tantrums.07:06
jtv1Hmm... actually I think I'll hold off.  I'd like to see the tantrums.07:07
jtv1stub: I haven't reviewed code from you for a while...  the difference in coding style is like night and day.07:49
jtv1stub: a few small things though: line 60, in the list of date/time formats, we require a trailing comma.07:51
stubThat is either coffee intake, or that this is greenfields rather than extending existing code. Most of the time I seems to be wading about the guts of something horrible.07:51
stub(ie. code I wrote 5 years ago)07:51
jtv1Ah yes... as I keep trying to tell people, we can argue all we want about the intricacies of deterministic coding style, but existing code is a much stronger persuader than guideline #1728.317 sub b.07:52
jtv1I once tried to implement my own optparser types as well, though wasn't very successful at it... it'd be really nice to have our own little library of option types, e.g. DistroSeries.07:54
stubThat code is pretty much verbatim from the Python reference07:55
jtv1ah so07:55
stubThere is an example under 'extending optparse'07:55
jtv1On an unrelated note: in pageperformancereport.py, everything needs docstrings.07:55
stubCan I leave them until later? Main goal of this particular landing is to get the URL logged in the trace logs07:56
stub(I can land just that change if necessary).07:56
jtv1Later never comes, does it?07:56
stubI'm still tweaking that script - just added support for compressed logs and malformed lines.07:57
stubTypeError: A() takes exactly 4 arguments (149 given)07:57
jtv1Very helpful...07:58
jtv1The docstrings can be quite short, as long as they place things in context.  F'r instance, Category could do with a one-liner "Category of <whatever it is you're categorizing>."07:59
stubOk. I'll add docstrings.07:59
jtv1Thanks.  Also makes reviewing easier.07:59
jtv1Another thing: scripts don't normally follow the entire python file template AFAIK...  for one, they start out by importing _pythonpath.08:00
jtv1BTW is it worth reporting the number of malformed lines, to help detect problems in recognizing record types?08:03
jtv1Line 365, instead of "for i in range(0, len(histograms)): histogram = histograms[i]" use "for i, histogram in enumerate(histograms)"08:05
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
jtv1stub: in "a list of Category, in report order", what does "in report order" mean?08:25
stubThe order we want things displayed in the report.08:25
jtv1stub: so that's why you wanted an order-preserving config?08:26
stubYes. I need to add an order= parameter to the config or a different config format entirely so we can put important stuff first.08:27
stubAnd group similar areas etc.08:27
jtv1I see... good call on leaving it out, I guess08:27
jtv1stub: I think parse() would be more manageable if at least the loop for reading one file was in a separate function...  less indentation, but also you won't need the "continue # skip to next line" and "break # skip to next file" stuff08:31
stubjtv1: Then I lose an entry that spans two log files08:32
stub(although maybe that doesn't matter for statistical analysis)08:32
jtv1you can have entries that span two log files!?08:32
jtv1Frankly though I don't see how you'd lose anything at all by doing it that way08:33
jtv1Unless you've got some loop-carried variable definitions hidden in there, which would be icky.08:34
stubYour right. I don't lose.08:37
jtv1btw I now see that "requests" only holds ongoing requests...  it'd have helped to make that clear.08:37
jtv1stub: what does the "$(" before the JS do?  I don't think you're closing that parenthesis.08:43
stubjtv1: Some jquery thing I don't understand. The basic structure was cargo culted from examples.08:44
jtv1stub: I'm getting old.  I find myself tempted to ask "wouldn't it be easier to do this with XSLT?"08:46
stubThe answer to that question is almost always 'no'. And when you are on a team where XSLT knowledge is nearly zero, it is almost certainly 'no'.08:47
jtv1Maybe we're all getting old.08:48
jtv1Anyway, please make sure that parenthesis is closed.08:48
jtv1Oww, you're importing main into a script elsewhere.  In that case, it should've been in the __all__ where you export it.08:49
stubI need to replace parsedt with something that isn't broken (it fails when there are no milliseconds)08:50
* jtv1 says Hail Mary in penance for assuming that it'd be called where it was defined.08:50
jtv1You need to test stuff as well.  We live in Thailand; we don't want to fill our even virtual toolboxes with utensils that fall apart when we finally need them.08:51
* jtv1 had another "most solid-looking in the shop" piece of crap spontaneously fall apart after about a week08:52
jtv1Made out of solid, chromed steel—except the moving parts which are chromed plastic.08:52
jtv1stub: about the config... that's for production only?09:02
stubIts an example for now. The report is still a work in progress really. People will use their own configs for what reports they want to generate.09:03
stubBut we need the logging bit landed so we can get some real data to test the report script against. 09:04
stubEven without the URLs, I'm finding lots of issues with the parser I'm having to fix now.09:05
stub(using the existing trace files, sans URLs with virtual host information)09:05
jtv1stub: I have to go do stuff now... should give you some time to do that.  :-)09:20
stubI won't have time to sort that today. I want to land the logging change (the important bit I want to land so I have logs to test against on monday, allowing me to complete the report script)09:23
=== matsubara-afk is now known as matsubara
=== kiko` is now known as kiko
=== salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
stubsalgado: https://code.edge.launchpad.net/~stub/launchpad/trivial/+merge/18258 (trivial)12:07
=== mrevell is now known as mrevell-lunch
jtvstub: re-doing the whole thing?13:08
stubThe report will change enough. Changed a bit already.13:08
stubThe report isn't the important thing though since it is a work in progress13:09
stubAnd there is little point polishing it until we can run some real data through it. I'm just working on memory issues now.13:09
=== mrevell-lunch is now known as mrevell
jtvrockstar: "nag"13:47
=== matsubara is now known as matsubara-lunch
=== salgado is now known as salgado-lunch
bacbeuno: we talked about this new portlet earlier in the week and it is my understanding you were happy with it.  could you mark a UI review on it?  https://code.edge.launchpad.net/~bac/launchpad/bug-490518-link-suggestion/+merge/1804015:08
beunobac, sure, ine sec15:16
=== matsubara-lunch is now known as matsubara
=== salgado-lunch is now known as salgado
=== EdwinGrubbs is now known as Edwin-lunch
=== gary_poster is now known as gary-lunch
=== jml is now known as joneaux
bacbeuno: got a sec yet?  :)18:36
=== joneaux is now known as jml
beunobac, ah18:37
beunoit seems I lied18:37
beunoreally opening up the MP this time18:37
bacstretched the truth18:37
bachad good intentions18:37
beunobac, any screenshots I can look at?18:38
bacbeuno: just the same old ones from earlier.  let me dig them up18:38
bacbeuno: recall the spacing issues are a webkit problem and a bug has been filed for it18:39
beunoI remember now18:39
bacyou the rock18:40
beunoI think it's more of a balad today18:40
=== salgado changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
rockstarsalgado, A review. I am needing one.19:16
salgadorockstar, how big is it?  I'll need to leave soon19:17
rockstarsalgado, it's not very big, but is a big priority.19:18
salgadorockstar, ok, where is it?19:18
rockstarsalgado, it's the branch to fix the problems detailed in abentley's Root Cause Analysis email.19:18
rockstarsalgado, finishing letter now.19:18
rockstarsalgado, https://code.edge.launchpad.net/~rockstar/launchpad/scanner-events/+merge/1828119:24
=== gary-lunch is now known as gary_poster
salgadorockstar, care to tell me a bit about IBranchScanJobSource and how it is tied together to the rest of the branch scanning system?19:36
rockstarsalgado, so there is the IBranchScanJob and IBranchScanJobSource - I'm not sure why we have two interfaces, but one is for instances usually, and the other is for classmethods.19:40
rockstarIBranchScanJobSource specifically gives you all the methods you need to get all the BranchScanJobs, etc.19:40
abentleyrockstar: The two interfaces are because stateful things don't really work as utilities.19:41
derycksalgado, I have one for you, as you have time, please.19:41
=== deryck changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
salgadoderyck, I won't have time today, sorry.  reviewing one for rockstar now and I'll have to leave after that19:42
derycksalgado, no worries.19:42
derycksalgado, thanks anyway!19:42
rockstarAll BranchJob types have a wrapper around them that provides extra information.  BranchScanJob is the class in this case, and it has no extra information needed.19:42
=== deryck changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: jkakar(storm) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
deryckyo rockstar !19:42
rockstarderyck, I was just about to offer.19:42
deryckyou da man!19:42
deryckrockstar, https://code.launchpad.net/~deryck/launchpad/not-implemented-errors/+merge/1828319:42
=== salgado changed the topic of #launchpad-reviews to: on-call: - || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
salgadorockstar, ISTM that .run() now expects self.server to be not-None, but do we have anything to make sure contextManager() is called before run(), to make sure self.server is set?19:44
rockstarsalgado, it's actually cls.server, but yes, the JobCronScript deals with all of that.19:45
rockstarsalgado, it even uses the context manager stuff that barry posted to the list about.19:45
salgadorockstar, would it be worth having an "assert self.server is not None" in run(), just to make it clear?19:46
rockstarsalgado, well, run is called in a with statement, so it would bomb out before then.19:47
salgadoalso, what do you think of a test showing that all the expected subscribers are actually registered for the event?  the existing one only shows (implicitly) that the subscriber for generating diffs is registered, but not the others19:48
rockstarAIUI, the with is what handles the contextManager call, and knows how to clean up after itself.19:48
rockstarsalgado, well, I don't think this fix is the way forward.  abentley, thumper, and I are all in agreement there.  It's the how we're not quite sure of.19:49
abentleysalgado: This isn't a nice fix, this is a quick, minimally-invasive fix.19:49
salgadook, maybe that'll be clear for someone reading the whole thing and not just the diff, but maybe a comment explaining how we know self.server will be set before run() is called would be nice19:49
rockstarsalgado, maybe, but this isn't the only place that uses the contextManager.19:50
salgadofair enough, I leave that up to you19:50
rockstarIf you're implementing JobCronScript, you need to know about contextManager and how it works.19:50
salgadobut if you're just reading the code, it might take you longer than necessary to understand it19:51
rockstarderyck, r=me19:51
deryckrockstar, you rock!  thanks19:51
salgadorockstar, well, since this is more like a quick fix, then, I guess I'm happy with it.  my only real concern was with the test, but I see you guys plan to address that19:53
rockstarsalgado, yes, we definitely plan to address it.  We'll need to chat with gary about it though.  I'm sure he'll have some good insights.19:54
* gary_poster likes chatting :-)19:55
abentleygary_poster: I think this is what jml wanted to talk with you about, too.  Basically, we want to make sure we're using Zope events the best way for us.19:56
rockstargary_poster, you're probably also hating packing right now.  :)19:56
gary_posterabentley: gotcha.  rockstar: eh, I'll throw whatever in a suitcase.  More worried that the predicted 10 inches of snow will bring NC to a standstill tomorrow. :-)19:57
rockstargary_poster, 10 in is NC? You bet it will.19:57
rockstarMaybe you can convince bac to give you a ride in one of his Land Rovers.  :)19:57
gary_posteryeah.  I'm hoping it won't really do that.  If it does, I may have an extra day at home19:57
bacgary_poster: i'll come get you!19:59
bacrockstar: you taunt.  you know only one works...20:00
rockstarbac, yea, and it sounds like the floor might be suspect.  :)20:00
bacrockstar: nah, the floor is mostly fine.  it's the frame.  :(20:00
=== salgado is now known as salgado-afk
gary_posterbac, ha, don't offer too often or I might take you up on it ;-)  I've been wondering if there were a we'll-drive-in-snow taxi service around here.  Depending on how bad things look I might look into it.  Not keen on piling the family in to a NC snow drive to the airport if things look bad.20:02
bacah, i forgot you have to fly out20:03
bacgary_poster: the new terminal looks very comfortable for spending the night if you get snowed in20:04
gary_posterbac, thanks ;-)20:05
=== matsubara is now known as matsubara-afk
=== Edwin-lunch is now known as EdwinGrubbs

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!