Good point. But it raises the question, is there any point to the new overload of TSHttpHdrStatusSet(). If the transaction handle is available anyway, it would be better to make the shorter call to TSHttpTxnStatusSet(), no?
On Monday, September 8, 2025 at 11:43:30 AM EDT, Brian Neradt <[email protected]> wrote: Thank you Walt for the suggestion. That's an interesting idea. I started working on a draft, but while it looks promising for the TSHttpTxnStatusSet version, I don't think this will work for the TSHttpHdrStatusSet version - that will need to take the txn (ultimately, a reference to the HttpSM) in order to store the plugin information, or file and line information, per your suggestion. Since those callsites will need to change anyway to pass the txn, I think it will be easier for ops if we record a human readable string for the setter (header_rewrite, ip_allow, etc.) for them. On Sat, Sep 6, 2025 at 12:24 PM Walt Karas <[email protected]> wrote: > An alternative approach is to make this change: > > #define TSHttpTxnStatusSet(TXNP, STATUS) TSHttpTxnStatusSetImpl((TXNP), > (STATUS), __FILE__, __LINE__) > > void TSHttpTxnStatusSetImpl(TSHttpTxn txnp, TSHttpStatus status, const > char *src_file, int src_line); > > And add the log fields "HTTP Status Set Source Code File" and "HTTP Status > Set Source Code Line". > > You can add a rump implementation of TSHttpTxnStatusSet(), with a > preceding #undef: > > #undef TSHttpTxnStatusSet > > void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status) > { > TSHttpTxnStatusSetImpl(txnp, status, "NEEDS REBUILD", 666); > } > > so that existing plugin binaries will remain usable. > > (Analogous changes for TSHttpHdrStatusSet().) > > > On Saturday, September 6, 2025 at 11:53:25 AM EDT, Brian Neradt < > [email protected]> wrote: > > Hi [email protected], > > At Yahoo, our operations staff frequently has difficulty determining what > component is setting a particular response, such as a 403. This can come > from ip_allow, header_rewrite, or a variety of internal plugins. I'm > submitting a proposal for a new log field, plss (plugin status setter) such > that they can make the identifier for the setter of the status a part of > the transaction log. > > To support this, however, we need the plugins to self-identify when they > set the status so the state machine knows the value for the new plss log > field when the transaction log is written. Here are the two APIs to set the > status: > > - void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status) > - TSReturnCode TSHttpHdrStatusSet(TSMBuffer bufp, TSMLoc offset, > TSHttpStatus status) > > The first can be augmented pretty trivially: it simply can be updated to > take a string since it already takes as a parameter the txn in which the > string can be stored. The second is a bit trickier since it takes a buffer > and modifies the status in place. However, most plugins will have the txn > readily available at the place where TSHttpHdrStatusSet is called, so I > propose adding parameters to it that take the txn and the string. Further, > for situations where it will be easier to set the status elsewhere, I > propose a new SetterSet API that takes the txn and the string. > > Thus, I propose adding (not modifying the above for ABI compatibility) the > following functions: > > - void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status, > std::string_view setter) > - TSReturnCode TSHttpHdrStatusSet(TSMBuffer bufp, TSMLoc offset, > TSHttpStatus status, TSHttpTxn txnp, std::string_view setter) > - std::string_view TSHttpStatusSetterGet(TSHttpTxn txnp) > - void TSHttpStatusSetterSet(TSHttpTxn txnp, std::string_view setter) > > Note that this will not break old builds since the old API will be > preserved. Also, the original API is still valid in some circumstances > since some plugins set and then re-set the status for cache-interaction > purposes - these will not want to record a setter since they aren't > ultimately changing the status. > > I've created a PR which demonstrates this and updates the plugins that set > the status with this new API: > > https://github.com/apache/trafficserver/pull/12484 > > Let me know whether there are any thoughts or concerns. > > Thank you. > > Brian > -- > "Come to Me, all who are weary and heavy-laden, and I will > give you rest. Take My yoke upon you and learn from Me, for > I am gentle and humble in heart, and you will find rest for > your souls. For My yoke is easy and My burden is light." > > ~ Matthew 11:28-30 > -- "Come to Me, all who are weary and heavy-laden, and I will give you rest. Take My yoke upon you and learn from Me, for I am gentle and humble in heart, and you will find rest for your souls. For My yoke is easy and My burden is light." ~ Matthew 11:28-30
