[GitHub] [trafficserver] traeak commented on a change in pull request #8395: Added metrics to the rate limit plugin and document the new options

[GitBox <gi...@apache.org>]

traeak commented on a change in pull request #8395:
URL: https://github.com/apache/trafficserver/pull/8395#discussion_r727124649



##########
File path: plugins/experimental/rate_limit/utilities.cc
##########
@@ -111,3 +111,48 @@ retryAfter(TSHttpTxn txnp, unsigned retry)
     }
   }
 }
+
+///////////////////////////////////////////////////////////////////////////////
+// Parse a URL to obtain a description for use with metrics when no user
+// provided tag is available. This is used by the remap side of the plugin,
+// while the SNI side uses the FQDN associated with each limiter instance
+// which is obtained from the list of SNIs in the global plugin configuration.
+//
+std::string
+getDescriptionFromUrl(const char *url)
+{
+  TSMBuffer const buf = TSMBufferCreate();
+  TSMLoc url_loc      = nullptr;
+
+  const int url_len = strlen(url);
+  std::string description;
+
+  if (TS_SUCCESS == TSUrlCreate(buf, &url_loc) && TS_PARSE_DONE == TSUrlParse(buf, url_loc, &url, url + url_len)) {
+    int host_len, scheme_len = 0;
+    const char *s  = TSUrlSchemeGet(buf, url_loc, &scheme_len);
+    const char *h  = TSUrlHostGet(buf, url_loc, &host_len);
+    const int port = TSUrlPortGet(buf, url_loc);
+
+    const std::string hostname = std::string(h, host_len);
+    const std::string scheme   = std::string(s, scheme_len);
+
+    TSDebug(PLUGIN_NAME, "scheme = %s, host = %s, port = %d", scheme.c_str(), hostname.c_str(), port);
+
+    description = scheme;
+    description.append("." + hostname);

Review comment:
       This will create a temporary which is then appended.  Might be better to split into 2 append calls.

##########
File path: plugins/experimental/rate_limit/limiter.h
##########
@@ -139,6 +172,50 @@ template <class T> class RateLimiter
     }
   }
 
+  void
+  initializeMetrics(uint type)
+  {
+    TSReleaseAssert(type < RATE_LIMITER_TYPE_MAX);
+    memset(_metrics, 0, sizeof(_metrics));
+
+    std::string metric_prefix = prefix;
+    metric_prefix.append("." + std::string(types[type]));
+
+    if (!tag.empty()) {
+      metric_prefix.append("." + tag);
+    } else if (!description.empty()) {
+      metric_prefix.append("." + description);
+    }
+
+    for (int i = 0; i < RATE_LIMITER_METRIC_MAX; i++) {
+      size_t metricsz = metric_prefix.length() + strlen(types[type]) + description.length() + 16; // padding for suffixes+dots
+      char *metric    = (char *)TSmalloc(metricsz);

Review comment:
       the pointer itself can be made const.

##########
File path: plugins/experimental/rate_limit/limiter.h
##########
@@ -139,6 +172,50 @@ template <class T> class RateLimiter
     }
   }
 
+  void
+  initializeMetrics(uint type)
+  {
+    TSReleaseAssert(type < RATE_LIMITER_TYPE_MAX);
+    memset(_metrics, 0, sizeof(_metrics));
+
+    std::string metric_prefix = prefix;
+    metric_prefix.append("." + std::string(types[type]));
+
+    if (!tag.empty()) {
+      metric_prefix.append("." + tag);
+    } else if (!description.empty()) {
+      metric_prefix.append("." + description);
+    }
+
+    for (int i = 0; i < RATE_LIMITER_METRIC_MAX; i++) {
+      size_t metricsz = metric_prefix.length() + strlen(types[type]) + description.length() + 16; // padding for suffixes+dots

Review comment:
       can probably const this.




-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org