Hi Aman,
On Sat, Mar 24, 2012 at 10:55:50PM -0700, Aman Gupta wrote:
> > Agreed. For this reason we've recently extended the log format to optionally
> > include new fields such as the outgoing source IP:port. I would not have
> > expected another request to come that soon :-)
>
> Quite the coincidence. I was working in a haproxy-1.4 checkout, so I
> missed the new logging commits.
OK just to be sure, you should *really* make your changes on 1.5-dev, not
1.4. 1.4 is in deep maintenance mode and I don't intend to merge such
changes there, as every time I did I caused some regressions.
> > This is really nice that you did that, as it's been on my todo list for
> > a long time. But I have comments on this before I can merge it :
>
> Awesome, thanks. This is my first time working with the haproxy
> codebase and I wasn't sure if I was on the right track. Definitely
> appreciate the feedback.
For someone who works for the first time on the code, you appear to
understand pretty well how it works, so feel free to continue :-)
> > 1) I'd rather use "debug sessions" than "show events" because till now
> > "show anything" only lists a finite list, and "events" can be a lot
> > of things.
>
> I was initially using "stream", then "events". I considered "show sess
> events" but "show events" was simpler to implement.
>
> I agree re-using the "show" prefix is confusing, so I'll switch to
> "debug sessions" (or "stream sessions"?)
The advantage with "debug" is that everyone is used to be very careful
when enabling debug, and to get a lot of information. And in fact, with
your patch we get some information similar to what we get when starting
haproxy with -d.
> > 2) it looks to me like the stats socket is still usable in parallel to the
> > command. If so, we could as well as an "undebug all" command to stop
> > the debugging. But I'm not requesting that for now, it's more out of
> > curiosity.
>
> I wasn't quite sure what to do here either. Initially, once streaming
> was enabled you would have to reconnect to do anything else. In the
> current implementation, if a new command is received (or invalid input
> with a newline), streaming is turned off and we go back to processing
> commands.
OK good to know. So basically we just have to press enter to stop this.
> Right now it doesn't make sense to use any other commands while
> streaming, because it would be possible to separate the output. But if
> we plan on adding more streaming/debug subcommands, then an "undebug
> all" makes a lot of sense.
The advantage with "undebug all" is that many network admins are used to
do that when they don't want to debug anymore :-)
But since it's not needed at the moment, let's keep that for later.
> While streaming, it probably makes sense to disable commands like
> "show sess" so they do not interfere with the streaming output.
Not really. In fact, if the command is run in interactive mode, the
user knows what he's doing. So he gets mangled output but understands
the event he was waiting for finally happened. For instance if you put
a filter on a specific source address, you might still want to use the
CLI while waiting for it.
> > I would personally like to see 3 steps instead of 2 per session :
> > - incoming session accepted
> > - forward to server
> > - closed session to client
> >
> > We could then have a single letter char instead of +/- which would report
> > the state (A/F/C) :
> >
> > A 1 127.0.0.1:50869 - 127.0.0.1:9418
> > F 1 127.0.0.1:50869 - 127.0.0.1:9418 | 127.0.0.1:50870 - 127.0.0.1:6000
> > C 1 127.0.0.1:50869 - 127.0.0.1:9418
>
> I like this.
>
> I'm not sure how far to go in this direction, though. It would be
> useful to include a lot of the details from the "show sess" command-
> at the very least frontend/backend/srv names. It probably also makes
> sense to include the id that "show sess <id>" uses, so you can lookup
> detailed session information from an incoming event.
>
> For my use case the bare minimum I needed was the proxied and
> disconnected events, so that's where I started.
And you did right. We just need to be careful about the syntax when
adding new information so that it does not break existing scripts.
> > Here it's a waste of CPU cycles to call getpeername/getsockname 4 times
> > for addresses we generally already have. You have everything in the
> > stream_interface's addr (.from, .to). The local address is not always
> > filled, you have to check the session's flags for SN_FRT_ADDR_SET and
> > SN_BCK_ADDR_SET and call the associated functions accordingly prior
> > to use the addresses. The advantage is that the getpeername/getsockname
> > are already performed once in all of a session's life.
>
> I saw cli_addr was available in the session, but wasn't sure about the
> others. I'll fix this up now that I know where the addresses are
> stored, and call getpeername only once when necessary.
It's in 1.5. You'll also notice that the flag is only set at one place
in tcp_connect_server(). You should just move that to a function
"get_bck_addr()" similar to what is done in get_frt_addr().
> > This is not appropriate in my opinion. 1) it's a specific use, and 2)
> > it's already possible to set the timeout on the stats command line,
> > so better increase the CLI timeout before starting the dump.
>
> Is it possible to use "set timeout cli" to set an infinite timeout?
No, the largest you can use is 24 days. But quite frankly, do you really
think it makes sense to wait for an even for that long ?
> I'm not convinced it makes sense to have a timeout when streaming, but
> if its possible to disable it beforehand I guess that's ok.
The problem is not to have a timeout when streaming, but to have a timeout
for any connection to an external process. When we later support CLI over
TCP/SSL for instance, it will be mandatory to support a timeout.
> >> out:
> >> DPRINTF(stderr, "%s@%d: st=%d, rqf=%x, rpf=%x, rql=%d, rqs=%d,
> >> rl=%d, rs=%d\n",
> >> __FUNCTION__, __LINE__,
> >> si->state, req->flags, res->flags, req->l, req->send_max,
> >> res->l, res->send_max);
> >>
> >> if (unlikely(si->state == SI_ST_DIS || si->state == SI_ST_CLO)) {
> >> + stats_event_listener_remove(s);
> >> /* check that we have released everything then unregister */
> >> stream_int_unregister_handler(si);
> >
> > Normally you don't need this, you just need to add a .release callback
> > in cli_applet (dumpstats.c) which does the work.
>
> I can't seem to find the release callback you're referring to.
You're right, the "si_applet" type does not have a release callback, it's
only in the stream interface. You can proceed as is done in peer_accept(),
by setting ->release after stream_int_register_handler() or you can
improve that by adding a release callback in the si_applet and make
stream_int_register_handler() automatically assign it to the stream
interface (preferred solution).
> > Thanks for doing this, it looks really appealing ! I'm impatient to
> > see what realtime info everyone will think about ! I suspect that
> > filtering on a frontend will be among the first requests in the
> > wish list, quickly followed by source-mask filtering !
>
> Thanks for taking the time to code review. I'm glad this is something
> you think is useful and worth merging upstream.
You're welcome.
> I did a lot of cleanup yesterday, to refactor and bring the code in
> line with the style guide. My branch is at
> https://github.com/tmm1/haproxy/compare/session-events (or
> https://github.com/tmm1/haproxy/compare/session-events.diff for just
> the patch).
>
> I'll incorporate your fixes above and email another patch for review.
That's perfect. Sending emails to the list is preferred over sending
links to external trees, as there are several persons able to review
code on the list and to suggest good ideas.
> As far as filtering, I already started on a fe/be filter with the
> "show events [<name>]" syntax.
Don't forget to prefix your <name> field by a special keyword (eg:
"frontend" or "proxy" or whatever you want), so that it leaves the
possibility to add new keywords later.
> This uses strcmp though, since I wasn't
> sure how best to store each user's filters.
> (https://github.com/tmm1/haproxy/compare/session-events...session-events-filter).
That's easy, just resolve the frontend name to a pointer using
findproxy(), and store the pointer in the stats context, otherwise
leave it NULL. In your checks, you'll just have to compare the
pointer with session->fe or session->be.
> Filtering by event type would also be interesting, especially as the
> number of events increases.
Yes of course.
> Finally, one thing I struggled with was the linked list of event
> listeners. It went through a couple iterations and I think I have it
> implemented sanely now, but if you could take a look and make sure I'm
> not doing anything stupid that would be great.
OK thanks for letting me know. At first glance it seemms OK, but I'll
review it more deeply with your next patch.
Regards,
Willy
