Yes. After chatting with Sudheer and Susan, I think this API is not the correct approach - I should look at adding session milestones in parallel to transaction milestones, and move the current TLS timing data there, along with the session start / end times. I'll take a look at how much work that would be. A preliminary glance indicates it wouldn't be too much work. I'll put this on hold until I verify the feasibility of using milestones.
On Fri, Sep 20, 2019 at 9:41 AM Sudheer Vinukonda <sudheervinuko...@yahoo.com.invalid> wrote: > Hey Alan, > On further thought, I think I actually agree and like what you are saying > about not mixing Session and TXN data together. We are already a bit > hampered by doing that for Access logs and have to duplicate context > around, which makes it harder to maintain (not to mention making the SM > context needlessly larger). > So, +1 to your proposal from my side :=) > Like we discussed on slack though, it looks like the Txn milestones also > record TLS Handshake start/stop times right now and it'd be nice to remove > them from there (may be for 10.x) for consistency. > And in general, we should keep the principle in mind of not mixing session > and txn data as much as feasible! > Thanks! > > > On Thursday, September 19, 2019, 03:38:57 PM PDT, Sudheer Vinukonda < > sudheervinuko...@yahoo.com> wrote: > > Ugh, I was typing half asleep and while in a meeting (thought you could > still read my mind though :) ) > Anyway, will this work? > > milestones[SSN_START] = ua_txn->get_proxy_ssn()->ssn_start_time; > > > On Thursday, September 19, 2019, 03:31:14 PM PDT, Alan Carroll > <solidwallofc...@verizonmedia.com.INVALID> wrote: > > No, that won't work - the start time is for the session, not the > transaction. > > On Thu, Sep 19, 2019 at 4:37 PM Sudheer Vinukonda > <sudheervinuko...@yahoo.com.invalid> wrote: > > > Of course, by automatic, I don't mean out of thin air :) - you just need > > to initialize it in the TXN milestones just like you'd do the rest of the > > TXN milestones. for e.g in HttpSM::init(). > > milestones[SSN_START] = Thread::get_hrtime(); > > The API doesn't need to do anything different, you just need the wiring > to > > fill the data in the SM. Again like I said, I'm not fully opposed to the > > new API, just saying it seems something that's relevant/related to > existing > > milestones as well and seemed like a natural fit. > > > > > > > > > > On Thursday, September 19, 2019, 02:07:29 PM PDT, Alan Carroll > > <solidwallofc...@verizonmedia.com.INVALID> wrote: > > > > Currently, the start time is stored in ProxySession. How would adding > > `SSN_START` to the milestones work automatically? The value would still > be > > stored in ProxySession and not in the milestones array. Would it be > copied > > out at the start of every transaction to the HttpSM local milestone > array? > > If not, then you need the special index check to handle `SSN_START` > > differently. It can't be store persistently in the milestones because > those > > are cleared every transaction. > > > > Again, there are already a number of API functions like this (see the PR, > > one thing it does is group these together). A few of them have overloads > > that work via the TXN object, but all of them are directly accessible > via a > > session object. I really didn't think this would be controversial given > > that we already do this, just not for this particular session property. > > > > > > > > On Thu, Sep 19, 2019 at 2:20 PM Sudheer Vinukonda > > <sudheervinuko...@yahoo.com.invalid> wrote: > > > > > Hmm..TXN has access to the session it's associated with and I > personally > > > don't feel it'd look ugly or bad to combine session related data to a > TXN > > > milestone API (we already do that in Access logs). > > > The existing API (`sm->milestones[milestone]` ) should just work > > > automatically, if `SSN_START_TIME` is added to `TSMileStonesType`. May > be > > > I'm missing something, but, I can't think of any special checks needed. > > > That said, while the advantage would be one less API to support and > > > consistency in terms of leveraging existing API, the major disadvantage > > > though would be that we'd always need to have a TXN to look at the > > Session > > > start time. > > > I don't know if there's any specific use case that needs to look at > > > Session Start time outside of a Txn, but, I won't be surprised if there > > is > > > and it does seem like a reasonable thing to support though. > > > I'm +0 on adding `TSHttpSsnStartTime`, but, would like to hear from > > others > > > too on what they think. > > > > > > On Thursday, September 19, 2019, 11:58:32 AM PDT, Alan Carroll > > > <solidwallofc...@verizonmedia.com.INVALID> wrote: > > > > > > It's not really associated with the Transaction, it's a session > > property. > > > It would be somewhat ugly to implement that way, as you would then need > > to > > > do special checks on the milestone enum to handle the session start > time > > > differently, since it's in a completely different place. The code for > > this > > > function is effectively "return > > > reinterpret_cast<ProxySession*>(ssnp)->ssn_start_time;". > > > > > > It would also seem a bit odd to have things like the session VConn or > ID > > > available via the milestone interface. > > > > > > On Thu, Sep 19, 2019 at 1:46 PM Sudheer Vinukonda > > > <sudheervinuko...@yahoo.com.invalid> wrote: > > > > > > > Hmm...Would it make sense to add this as a milestone and extend the > > > > existing API `TSHttpTxnMileStoneGet` (and `TSMilestonesType`) to > > support > > > > session start time and possibly other session level context > associated > > to > > > > that Txn? > > > > Thoughts? > > > > > > > > > > > > On Thursday, September 19, 2019, 11:38:47 AM PDT, Alan Carroll > > > > <solidwallofc...@verizonmedia.com.INVALID> wrote: > > > > > > > > I'm playing with some session stuff and I'd like to add this, since > > the > > > > data is already stored in the session, it's trivial to access. > > > > > > > > > >
