> On Jun 12, 2019, at 11:00 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
> wrote:
>
> What do you mean by "exposing diags"?
>
> On Wed, Jun 12, 2019 at 11:50 AM Bryan Call <bc...@apache.org> wrote:
>
>> I would rather see TSDebug be optimized and exposing diags instead of
>> creating a new type of debug.
For a while now, we’ve talked about making better APIs for conditionally
enabling diags per transaction / event basis. Such as, “If client_ip = x.y.z/24
and request contains “?walt=1”, turn on diags for http.*”. There’s some of this
in the code already, it was done by Uri if I recall. In addition, Yahoo
(Susan?) added some partial support for per-IP diagnostics, but I don’t think
we ended up with any APIs that lets you control it in any reasonable way.
I agree with Bryan, improving on TSDebug() / Debug() (core) / TSDebugSpecific()
would be preferable. In fact, I wish that TSDebugSpecific() could go away,
merged into TSDebug/Debug() and a better API for programmatic control over when
diagnostics should be conditionally enabled.
Cheers,
— Leif
>>
>> -Bryan
>>
>>
>>> On Jun 12, 2019, at 9:44 AM, Walt Karas <wka...@verizonmedia.com.INVALID>
>> wrote:
>>>
>>> I don't believe so. The issue I'm trying to address is that there is
>> still
>>> significant execution time overhead for debug output calls when
>>> proxy.config.diags.debug.enabled is 0. This change would make the
>>> execution time overhead negligible (if TSFastDbg is used or TSDebug is
>>> changed to be like the proposed TSFastDbg). Another way of putting it
>> is,
>>> I want to clean up the existing TSDEBUG alternative to TSDebug.
>>>
>>> On Wed, Jun 12, 2019 at 11:31 AM Bryan Call <bc...@apache.org> wrote:
>>>
>>>> How would this be controlled on the command line when running
>>>> trafic_server -T?
>>>>
>>>> It looks like it is a departure from how debug tags and controlled now.
>>>>
>>>> -Bryan
>>>>
>>>>
>>>>> On Jun 11, 2019, at 1:21 PM, Walt Karas <wka...@verizonmedia.com
>> .INVALID>
>>>> wrote:
>>>>>
>>>>> Any +1's for simply replacing TSDebug with the proposed TSFastDbg ?
>>>>>
>>>>> On Mon, Apr 8, 2019 at 6:20 PM Leif Hedstrom <zw...@apache.org> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Apr 4, 2019, at 8:05 AM, Walt Karas <wka...@verizonmedia.com
>>>> .INVALID>
>>>>>> wrote:
>>>>>>
>>>>>> Because senility. probably "on" should be a non-pointer of type const
>>>>>> volatile char, and InkAPI in the core can just keep a pointer to the
>>>> whole
>>>>>> structure for updating.
>>>>>>
>>>>>> But I'm leaning more to fixing up the capability that already kinda
>>>> there.
>>>>>> Which is optimizimg only for the case when all debug output is
>> disabled.
>>>>>> Especially since maybe zwoop would be on my side for once :o) .
>>>>>>
>>>>>>
>>>>>>
>>>>>> My bad, whatever Walt said, I take the opposite stance.
>>>>>>
>>>>>> — leif
>>>>>>
>>>>>> P.s
>>>>>> J/K!
>>>>>>
>>>>>>
>>>>>> On Thu, Apr 4, 2019 at 8:51 AM Alan Carroll
>>>>>> <solidwallofc...@verizonmedia.com.invalid> wrote:
>>>>>>
>>>>>> This could enable a significant speed up for debug tags. One point is
>>>> that
>>>>>> when the debug tag string is set, at that point the debug objects
>> could
>>>> be
>>>>>> updated to the correct state according to the debug tag string, rather
>>>> than
>>>>>> checking it every time a debug message is logged.
>>>>>>
>>>>>> I have to ask, why is there a pointer to the enable flag, instead of
>>>> just
>>>>>> the enable flag? Your logic doesn't even check that flag, it checks
>>>> whether
>>>>>> the pointer is null.
>>>>>>
>>>>>> On Wed, Apr 3, 2019 at 11:52 AM Leif Hedstrom <zw...@apache.org>
>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Apr 3, 2019, at 10:47 AM, Walt Karas <wka...@verizonmedia.com
>>>>>>
>>>>>> .INVALID>
>>>>>>
>>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> It looks like the TS API already has less granular version of this:
>>>>>>
>>>>>> extern int diags_on_for_plugins;
>>>>>>
>>>>>> #define TSDEBUG \
>>>>>>
>>>>>> if (diags_on_for_plugins) \
>>>>>>
>>>>>> TSDebug
>>>>>>
>>>>>>
>>>>>> A problem with this is if you write:
>>>>>>
>>>>>>
>>>>>> if (flag)
>>>>>>
>>>>>> TSDEBUG(yadayada);
>>>>>>
>>>>>> else
>>>>>>
>>>>>> do_something();
>>>>>>
>>>>>>
>>>>>> then the else will be associated with the if hidden in the macro. I'd
>>>>>> prefer to change to something like:
>>>>>>
>>>>>>
>>>>>> extern const volatile char TS_detail_dbg_output_enabled;
>>>>>>
>>>>>>
>>>>>> #define TSFastDbg(_TAG, ...) \
>>>>>> do { \
>>>>>> if (TS_detail_dbg_output_enabled) \
>>>>>> TSDebug(_TAG, __VA_ARGS__); \
>>>>>>
>>>>>> } while(0)
>>>>>>
>>>>>>
>>>>>> Yeh, that kinda sounds like a bug, doesn’t it?
>>>>>>
>>>>>>
>>>>>> The negative is that we'd only see the benefit when all debug output
>>>>>>
>>>>>> was
>>>>>>
>>>>>> disabled. But, in any case, that is the main benefit, to avoid
>> impacts
>>>>>> during normal operation.
>>>>>>
>>>>>>
>>>>>> Right, we should always optimize for the common cases, obviously. When
>>>>>> there’s a decision to make, where you can either make it “equally”
>>>>>> expensive always, or more expensive in a rare case, but cheaper in the
>>>>>> common case, we should likely chose the latter.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Looking in src/tscore/Diags.cc, it looks like the debug enable flag is
>>>>>>
>>>>>> only
>>>>>>
>>>>>> set at startup time. So we wouldn't be able to enable TSFastDbg using
>>>>>> traffic_cntl without adding complexity.
>>>>>>
>>>>>> On Tue, Apr 2, 2019 at 5:48 PM Walt Karas <wka...@verizonmedia.com>
>>>>>>
>>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Add this new structure to apidefs.h:
>>>>>>
>>>>>> typedef struct TSFastDbgCntl_
>>>>>> {
>>>>>> const char * const tag; // nul-terminated string
>>>>>> const volatile char * const on; // pointer to 1-byte flag
>>>>>> }
>>>>>> TSFastDbgCntl;
>>>>>>
>>>>>> Add this new API function, that returns a pointer to an instance of
>>>>>>
>>>>>> the
>>>>>>
>>>>>> above structure:
>>>>>>
>>>>>> TSFastDbgCntl * TSCreateFastDbgCntl(const char *tag);
>>>>>>
>>>>>> Add this macro, which would be used in place of direct use of
>>>>>>
>>>>>> TSDebug().
>>>>>>
>>>>>>
>>>>>> #define TSFastDbg(_FD_CNTL, ...) \
>>>>>> do { \
>>>>>> if ((_FD_CNTL)->on) \
>>>>>> TSDebug((_FD_CNTL)->tag, __VA_ARGS__); \
>>>>>> } while(0)
>>>>>>
>>>>>> The first parameter to TSFastDbg() is a pointer that was returned by
>>>>>> TSCreateFastDbgCntl(). The remaining parameters are printf parameters
>>>>>> (format string and values matching format specifiers). The core would
>>>>>>
>>>>>> be
>>>>>>
>>>>>> responsible for changing the value of the 'on' fields in the
>>>>>>
>>>>>> TSFastDbgCntl
>>>>>>
>>>>>> instances. For direct calls to TSDebug(), even if the tag is
>>>>>>
>>>>>> disabled,
>>>>>>
>>>>>> all
>>>>>>
>>>>>> the parameters have to be prepared and pushed onto the stack. As was
>>>>>> discussed in IRC, you have to feel guilty about doing converting a
>>>>>> string_view to a string and outputting it with c_str(). Because the
>>>>>>
>>>>>> code
>>>>>>
>>>>>> to do this will be executed even if the debug tag is disabled.
>>>>>> Furthermore, for each call to TSDebug, it's necessary to do an
>>>>>>
>>>>>> associative
>>>>>>
>>>>>> lookup on the tag string to determine that output for the tag is
>>>>>>
>>>>>> disabled.
>>>>>>
>>>>>>
>>>>>> We would want to put equivalent capabilities in Diags.h in the core
>>>>>>
>>>>>> for
>>>>>>
>>>>>> debug output. The implementation is non-trivial, but I think it would
>>>>>>
>>>>>> take
>>>>>>
>>>>>> at most a week including the Au test additions.
>>>>>>
>>>>>> I looked at making TSFastDbg() an inline function rather than a macro:
>>>>>> https://godbolt.org/z/IfVbBk
>>>>>> The opitimization works well if you compile the code with the inline
>>>>>> function as C, but the optimization is poor if you compile it as C++.
>>>>>>
>>>>>> This
>>>>>>
>>>>>> difference exists for both gcc and clang. It's weird.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
