zwoop commented on code in PR #12316: URL: https://github.com/apache/trafficserver/pull/12316#discussion_r2178468845
########## doc/admin-guide/plugins/stats_over_http.en.rst: ########## @@ -41,7 +41,11 @@ default URL:: where host and port is the hostname/IP and port number of the server. +This plugin can also run as a remap plugin with an optional stats over http config file as a parameter:: + map / http://target @plugin=stats_over_http.so @pparam=config.file Review Comment: Nitpicky, but this seems like a dangerous example :). Shouldn't we use a more specific path here, like ``` map /_stats_over_http ... ``` or maybe even more specific (since the above captures the path for all hosts)? Also, maybe mention that the "target" doesn't imply / mean anything, and can be whatever ? ########## plugins/stats_over_http/stats_over_http.cc: ########## @@ -43,6 +43,7 @@ #include <zlib.h> #include <ts/remap.h> +#include <ts/remap_version.h> Review Comment: Separate issue, but seeing this, makes me thing we ought to add the define from this file into ts/remap.h. ########## plugins/stats_over_http/stats_over_http.cc: ########## @@ -800,10 +802,54 @@ stats_origin(TSCont contp, TSEvent /* event ATS_UNUSED */, void *edata) if (accept_encoding_field) { TSHandleMLocRelease(reqp, TS_NULL_MLOC, accept_encoding_field); } - TSHttpTxnReenable(txnp, reenable); + if (!is_remap) { + TSHttpTxnReenable(txnp, reenable); + } + return 0; } +static int +stats_origin_global_wrapper(TSCont contp, TSEvent, void *edata) +{ + config_t *config = get_config(contp); + return stats_origin(config, TS_EVENT_NONE, edata, false); +} + +TSReturnCode +TSRemapInit(TSRemapInterface *api_info, char *errbuf, int errbuf_size) +{ + CHECK_REMAP_API_COMPATIBILITY(api_info, errbuf, errbuf_size); + return TS_SUCCESS; +} + +TSReturnCode +TSRemapNewInstance(int argc, char *argv[], void **instance, char * /* errBuf ATS_UNUSED */, int /* errBufSize ATS_UNUSED */) +{ + config_holder_t *config_holder; + config_holder = new_config_holder(argc > 1 ? argv[2] : nullptr); + *instance = static_cast<void *>(config_holder); + /* Path was not set during load, so the param was not a config file, we also + have an argument so it must be the path, set it here. Otherwise if no argument + then use the default _stats path */ + if ((config_holder->config != nullptr) && (config_holder->config->stats_path.empty()) && (argc > 1) && + (config_holder->config_path == nullptr)) { + config_holder->config->stats_path = argv[2] + ('/' == argv[2][0] ? 1 : 0); + } else if ((config_holder->config != nullptr) && (config_holder->config->stats_path.empty())) { + config_holder->config->stats_path = DEFAULT_URL_PATH; Review Comment: Maybe I'm not reading this right, but shouldn't the default path here be the "from_url" path in the RRI (the remap.config "from URL") ? It kinda feels weird that you can override the path with the pparam or config file. What would happen if I do ``` map http://localhost/_stats_over_http http://default @plugin=stats_over_http.so @pparam=/bar ``` How could that ever match on the path now ? ########## plugins/stats_over_http/stats_over_http.cc: ########## @@ -800,10 +802,54 @@ stats_origin(TSCont contp, TSEvent /* event ATS_UNUSED */, void *edata) if (accept_encoding_field) { TSHandleMLocRelease(reqp, TS_NULL_MLOC, accept_encoding_field); } - TSHttpTxnReenable(txnp, reenable); + if (!is_remap) { + TSHttpTxnReenable(txnp, reenable); + } + return 0; } +static int +stats_origin_global_wrapper(TSCont contp, TSEvent, void *edata) +{ + config_t *config = get_config(contp); + return stats_origin(config, TS_EVENT_NONE, edata, false); +} + +TSReturnCode +TSRemapInit(TSRemapInterface *api_info, char *errbuf, int errbuf_size) +{ + CHECK_REMAP_API_COMPATIBILITY(api_info, errbuf, errbuf_size); + return TS_SUCCESS; +} + +TSReturnCode +TSRemapNewInstance(int argc, char *argv[], void **instance, char * /* errBuf ATS_UNUSED */, int /* errBufSize ATS_UNUSED */) +{ + config_holder_t *config_holder; + config_holder = new_config_holder(argc > 1 ? argv[2] : nullptr); + *instance = static_cast<void *>(config_holder); + /* Path was not set during load, so the param was not a config file, we also + have an argument so it must be the path, set it here. Otherwise if no argument + then use the default _stats path */ + if ((config_holder->config != nullptr) && (config_holder->config->stats_path.empty()) && (argc > 1) && + (config_holder->config_path == nullptr)) { + config_holder->config->stats_path = argv[2] + ('/' == argv[2][0] ? 1 : 0); + } else if ((config_holder->config != nullptr) && (config_holder->config->stats_path.empty())) { + config_holder->config->stats_path = DEFAULT_URL_PATH; + } + return TS_SUCCESS; +} + +TSRemapStatus +TSRemapDoRemap(void *id, TSHttpTxn rh, TSRemapRequestInfo * /* ATS_UNUSED */) Review Comment: Why `id` ? Typically we call this `ih` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
