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.
> > >
> >
>
