This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch NewAPIMetricsPOC in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 3a9e3ecd34ca1357b8629e3c81c80c6244c4abee Author: Leif Hedstrom <zw...@apache.org> AuthorDate: Wed Jul 5 17:18:31 2023 -0600 Initial cut at a new Metrics API --- configure.ac | 1 + include/api/Metrics.h | 166 ++++++++++++++++++++++++++++++++++++++ src/Makefile.am | 2 +- src/{ => api}/Makefile.am | 48 ++++++----- src/api/Metrics.cc | 100 +++++++++++++++++++++++ src/records/RecCore.cc | 5 ++ src/traffic_crashlog/Makefile.inc | 1 + src/traffic_layout/Makefile.inc | 1 + src/traffic_logcat/Makefile.inc | 3 +- src/traffic_logstats/Makefile.inc | 3 +- src/traffic_server/InkAPI.cc | 85 ++++--------------- src/traffic_server/Makefile.inc | 1 + 12 files changed, 321 insertions(+), 95 deletions(-) diff --git a/configure.ac b/configure.ac index 555a8a7baa..fd4690c137 100644 --- a/configure.ac +++ b/configure.ac @@ -2401,6 +2401,7 @@ AC_CONFIG_FILES([ src/tscpp/util/Makefile src/tscore/Makefile src/records/Makefile + src/api/Makefile tools/Makefile tools/trafficserver.pc tools/tsxs diff --git a/include/api/Metrics.h b/include/api/Metrics.h new file mode 100644 index 0000000000..aa7ab61168 --- /dev/null +++ b/include/api/Metrics.h @@ -0,0 +1,166 @@ +/** @file + + A brief file description + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#pragma once + +#include <array> +#include <unordered_map> +#include <tuple> +#include <mutex> +#include <thread> +#include <atomic> +#include <cstdint> + +#include "records/P_RecDefs.h" + +namespace ts +{ +class Metrics +{ +private: + using self_type = Metrics; + using IdType = int32_t; + using AtomicType = std::atomic<int64_t>; + +public: + static constexpr uint16_t METRICS_MAX_BLOBS = 2048; + static constexpr uint16_t METRICS_MAX_SIZE = 8192; // For a total of 16M metrics + static constexpr IdType NOT_FOUND = INT32_MIN; // <16-bit,16-bit> = <blob-index,offset> + +private: + using NameAndId = std::tuple<std::string, IdType>; + using NameContainer = std::array<NameAndId, METRICS_MAX_SIZE>; + using AtomicContainer = std::array<AtomicType, METRICS_MAX_SIZE>; + using MetricStorage = std::tuple<NameContainer, AtomicContainer>; + using MetricBlobs = std::array<MetricStorage *, METRICS_MAX_BLOBS>; + using LookupTable = std::unordered_map<std::string_view, IdType>; + +public: + Metrics(const self_type &) = delete; + self_type &operator=(const self_type &) = delete; + Metrics &operator=(Metrics &&) = delete; + Metrics(Metrics &&) = delete; + + virtual ~Metrics() = default; + + Metrics() { _addBlob(); } + + // Singleton + static Metrics &getInstance(); + + IdType newMetric(const std::string_view name); + IdType lookup(const std::string_view name); + AtomicType *lookup(IdType id) const; + + // Inlined, for performance + int64_t + increment(IdType id, uint64_t val = 1) + { + auto metric = lookup(id); + + return (metric ? metric->fetch_add(val) : NOT_FOUND); + } + + int64_t + decrement(IdType id, uint64_t val = 1) + { + auto metric = lookup(id); + + return (metric ? metric->fetch_sub(val) : NOT_FOUND); + } + + int64_t + get(IdType id) const + { + auto metric = lookup(id); + + return (metric ? metric->load() : NOT_FOUND); + } + + void + set(IdType id, int64_t val) + { + auto metric = lookup(id); + + if (metric) { + metric->store(val); + } + } + + bool + valid(IdType id) const + { + std::tuple<uint16_t, uint16_t> idx = _splitID(id); + + return (id >= 0 && std::get<0>(idx) < _cur_blob && std::get<1>(idx) < METRICS_MAX_SIZE); + } + + void + recordsDump(RecDumpEntryCb callback, void *edata) + { + return; + int16_t blob_ix, off_ix; + int16_t off_max = METRICS_MAX_SIZE; + + { + std::lock_guard<std::mutex> lock(_mutex); + + // Capture these while protected, in case the blobs change + blob_ix = _cur_blob; + off_ix = _cur_off; + } + + for (int i = 0; i <= blob_ix; ++i) { + auto blob = _blobs[i]; + auto &names = std::get<0>(*blob); + auto &metrics = std::get<1>(*blob); + RecData datum; + + if (i == blob_ix) { + off_max = off_ix; + } + for (int j = 0; j < off_max; ++j) { + datum.rec_int = metrics[j].load(); + // ToDo: The recordtype here is fine for now, but we should probably make this generic + callback(RECT_PLUGIN, edata, 1, std::get<0>(names[i]).c_str(), TS_RECORDDATATYPE_INT, &datum); + } + } + } + +private: + static constexpr std::tuple<uint16_t, uint16_t> + _splitID(IdType value) + { + return std::make_tuple(static_cast<uint16_t>(value >> 16), static_cast<uint16_t>(value & 0xFFFF)); + } + + void _addBlob(); + + std::mutex _mutex; + LookupTable _lookups; + MetricBlobs _blobs; + uint16_t _cur_blob = 0; + uint16_t _cur_off = 0; +}; // class Metrics + +} // namespace ts diff --git a/src/Makefile.am b/src/Makefile.am index 8e95c0c4a9..e6aaddec1d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -23,7 +23,7 @@ TESTS = lib_LTLIBRARIES = noinst_PROGRAMS = -SUBDIRS = tscpp/api +SUBDIRS = tscpp/api api if BUILD_WCCP SUBDIRS += wccp diff --git a/src/Makefile.am b/src/api/Makefile.am similarity index 58% copy from src/Makefile.am copy to src/api/Makefile.am index 8e95c0c4a9..4a78c0741d 100644 --- a/src/Makefile.am +++ b/src/api/Makefile.am @@ -1,3 +1,4 @@ +# libts Makefile.am # # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file @@ -17,28 +18,31 @@ include $(top_srcdir)/mk/tidy.mk -bin_PROGRAMS = -check_PROGRAMS = -TESTS = -lib_LTLIBRARIES = -noinst_PROGRAMS = - -SUBDIRS = tscpp/api - -if BUILD_WCCP -SUBDIRS += wccp -include traffic_wccp/Makefile.inc -endif - -include traffic_cache_tool/Makefile.inc -include traffic_via/Makefile.inc -include traffic_top/Makefile.inc -include traffic_server/Makefile.inc -include traffic_logstats/Makefile.inc -include traffic_crashlog/Makefile.inc -include traffic_layout/Makefile.inc -include traffic_logcat/Makefile.inc -include traffic_ctl/Makefile.inc +# check_PROGRAMS = test_api + +# TESTS_ENVIRONMENT = LSAN_OPTIONS=suppressions=$(abs_top_srcdir)/ci/asan_leak_suppression/unit_tests.txt +# TESTS = $(check_PROGRAMS) + +AM_CPPFLAGS += \ + @SWOC_INCLUDES@ \ + $(TS_INCLUDES) + +noinst_LIBRARIES = libtsapi.a + +libtsapi_a_SOURCES = \ + Metrics.cc + +# test_tsapi_CPPFLAGS = $(AM_CPPFLAGS)\ +# -I$(abs_top_srcdir)/lib/catch2 + +# test_tsapi_CXXFLAGS = -Wno-array-bounds $(AM_CXXFLAGS) +# test_tsapi_LDADD = libtsapi.la @SWOC_LIBS@ +# test_tsapi_SOURCES = \ +# unit_tests/test_Metrics.cc + +# clean-local: +# rm -f ParseRulesCType ParseRulesCTypeToLower ParseRulesCTypeToUpper + clang-tidy-local: $(DIST_SOURCES) $(CXX_Clang_Tidy) diff --git a/src/api/Metrics.cc b/src/api/Metrics.cc new file mode 100644 index 0000000000..a95588c172 --- /dev/null +++ b/src/api/Metrics.cc @@ -0,0 +1,100 @@ +/** @file + + The implementations of the Metrics API class. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "ts/ts.h" +#include "api/Metrics.h" + +namespace ts +{ + +// This is the singleton instance of the metrics class. +Metrics & +Metrics::getInstance() +{ + static ts::Metrics _instance; + + return _instance; +} + +void +Metrics::_addBlob() // The mutex must be held before calling this! +{ + auto blob = new Metrics::MetricStorage(); + + ink_assert(blob); + ink_assert(_cur_blob < Metrics::METRICS_MAX_BLOBS); + + _blobs[_cur_blob++] = blob; + _cur_off = 0; +} + +Metrics::IdType +Metrics::newMetric(std::string_view name) +{ + std::lock_guard<std::mutex> lock(_mutex); + auto it = _lookups.find(name); + + if (it != _lookups.end()) { + return it->second; + } + + Metrics::IdType id = (_cur_blob << 16 | _cur_off); + Metrics::MetricStorage *blob = _blobs[_cur_blob]; + Metrics::NameContainer &names = std::get<0>(*blob); + Metrics::AtomicContainer &atomics = std::get<1>(*blob); + + atomics[_cur_off].store(0); + names[_cur_off] = std::make_tuple(std::string(name), id); + _lookups.emplace(std::get<0>(names[_cur_off]), id); + + if (++_cur_off >= Metrics::METRICS_MAX_SIZE) { + _addBlob(); // This resets _cur_off to 0 as well + } + + return id; +} + +Metrics::IdType +Metrics::lookup(const std::string_view name) +{ + std::lock_guard<std::mutex> lock(_mutex); + auto it = _lookups.find(name); + + if (it != _lookups.end()) { + return it->second; + } + + return NOT_FOUND; +} + +Metrics::AtomicType * +Metrics::lookup(IdType id) const +{ + std::tuple<uint16_t, uint16_t> idx = _splitID(id); + Metrics::MetricStorage *blob = _blobs[std::get<0>(idx)]; + Metrics::AtomicContainer &atomics = std::get<1>(*blob); + + return &(atomics[std::get<1>(idx)]); +} + +} // namespace ts diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index 8fa774aadd..1bf0bd43fe 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -34,6 +34,7 @@ #include "records/P_RecUtils.h" #include "tscore/I_Layout.h" #include "tscpp/util/ts_errata.h" +#include "api/Metrics.h" // This is needed to manage the size of the librecords record. It can't be static, because it needs to be modified // and used (read) from several binaries / modules. @@ -1062,6 +1063,10 @@ RecDumpRecords(RecT rec_type, RecDumpEntryCb callback, void *edata) rec_mutex_release(&(r->lock)); } } + // Also dump the new ts::Metrics if asked for + if (rec_type & TS_RECORDTYPE_PLUGIN) { + ts::Metrics::getInstance().recordsDump(callback, edata); + } } void diff --git a/src/traffic_crashlog/Makefile.inc b/src/traffic_crashlog/Makefile.inc index 6fbf0b5416..77c9ab9024 100644 --- a/src/traffic_crashlog/Makefile.inc +++ b/src/traffic_crashlog/Makefile.inc @@ -42,4 +42,5 @@ traffic_crashlog_traffic_crashlog_LDADD = \ $(top_builddir)/iocore/net/libinknet.a \ $(top_builddir)/src/tscore/libtscore.la \ $(top_builddir)/src/tscpp/util/libtscpputil.la \ + $(top_builddir)/src/api/libtsapi.a \ @HWLOC_LIBS@ @SWOC_LIBS@ @YAMLCPP_LIBS@ diff --git a/src/traffic_layout/Makefile.inc b/src/traffic_layout/Makefile.inc index 0bdde7f6a0..cdc8f72789 100644 --- a/src/traffic_layout/Makefile.inc +++ b/src/traffic_layout/Makefile.inc @@ -46,4 +46,5 @@ traffic_layout_traffic_layout_LDADD = \ $(top_builddir)/src/records/librecords_p.a \ $(top_builddir)/src/tscore/libtscore.la \ $(top_builddir)/src/tscpp/util/libtscpputil.la \ + $(top_builddir)/src/api/libtsapi.a \ @SWOC_LIBS@ @HWLOC_LIBS@ @YAMLCPP_LIBS@ @LIBLZMA@ diff --git a/src/traffic_logcat/Makefile.inc b/src/traffic_logcat/Makefile.inc index c9c5a09dca..cb23da73f0 100644 --- a/src/traffic_logcat/Makefile.inc +++ b/src/traffic_logcat/Makefile.inc @@ -47,7 +47,8 @@ traffic_logcat_traffic_logcat_LDADD = \ $(top_builddir)/iocore/utils/libinkutils.a \ $(top_builddir)/src/tscore/libtscore.la \ $(top_builddir)/src/tscpp/util/libtscpputil.la \ - $(top_builddir)/mgmt/utils/libutils_p.la + $(top_builddir)/mgmt/utils/libutils_p.la \ + $(top_builddir)/src/api/libtsapi.a traffic_logcat_traffic_logcat_LDADD += \ @SWOC_LIBS@ @HWLOC_LIBS@ \ diff --git a/src/traffic_logstats/Makefile.inc b/src/traffic_logstats/Makefile.inc index 7a2ac7f0d1..5f1e40b545 100644 --- a/src/traffic_logstats/Makefile.inc +++ b/src/traffic_logstats/Makefile.inc @@ -52,7 +52,8 @@ traffic_logstats_traffic_logstats_LDADD = \ $(top_builddir)/iocore/utils/libinkutils.a \ $(top_builddir)/src/tscore/libtscore.la \ $(top_builddir)/src/tscpp/util/libtscpputil.la \ - $(top_builddir)/mgmt/utils/libutils_p.la + $(top_builddir)/mgmt/utils/libutils_p.la \ + $(top_builddir)/src/api/libtsapi.a traffic_logstats_traffic_logstats_LDADD += \ @SWOC_LIBS@ \ diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index a58bb29981..157f2c8929 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -34,6 +34,7 @@ #include "tscore/I_Layout.h" #include "tscore/I_Version.h" #include "tscore/Diags.h" +#include "api/Metrics.h" #include "InkAPIInternal.h" #include "Log.h" @@ -102,10 +103,6 @@ extern AppVersionInfo appVersionInfo; -// Globals for new librecords stats -static int api_rsb_index; -static RecRawStatBlock *api_rsb; - /** Reservation for a user arg. */ struct UserArg { @@ -752,11 +749,7 @@ sdk_sanity_check_null_ptr(void const *ptr) static TSReturnCode sdk_sanity_check_stat_id(int id) { - if (id < 0 || id >= api_rsb->max_stats) { - return TS_ERROR; - } - - return TS_SUCCESS; + return (ts::Metrics::getInstance().valid(id) ? TS_SUCCESS : TS_ERROR); } static TSReturnCode @@ -1829,18 +1822,6 @@ api_init() lifecycle_hooks = new LifecycleAPIHooks; global_config_cbs = new ConfigUpdateCbTable; - int api_metrics = max_records_entries - REC_INTERNAL_RECORDS; - if (api_metrics > 0) { - api_rsb = RecAllocateRawStatBlock(api_metrics); - if (nullptr == api_rsb) { - Warning("Can't allocate API stats block"); - } else { - Debug("sdk", "initialized SDK stats APIs with %d slots", api_metrics); - } - } else { - api_rsb = nullptr; - } - // Setup the version string for returning to plugins ink_strlcpy(traffic_server_version, appVersionInfo.VersionStr, sizeof(traffic_server_version)); // Extract the elements. @@ -7511,40 +7492,9 @@ TSCacheScan(TSCont contp, TSCacheKey key, int KB_per_second) int TSStatCreate(const char *the_name, TSRecordDataType the_type, TSStatPersistence persist, TSStatSync sync) { - int id = ink_atomic_increment(&api_rsb_index, 1); - RecRawStatSyncCb syncer = RecRawStatSyncCount; - - // TODO: This only supports "int" data types at this point, since the "Raw" stats - // interfaces only supports integers. Going forward, we could extend either the "Raw" - // stats APIs, or make non-int use the direct (synchronous) stats APIs (slower). - if ((sdk_sanity_check_null_ptr((void *)the_name) != TS_SUCCESS) || (sdk_sanity_check_null_ptr((void *)api_rsb) != TS_SUCCESS) || - (id >= api_rsb->max_stats)) { - return TS_ERROR; - } - - switch (sync) { - case TS_STAT_SYNC_SUM: - syncer = RecRawStatSyncSum; - break; - case TS_STAT_SYNC_AVG: - syncer = RecRawStatSyncAvg; - break; - case TS_STAT_SYNC_TIMEAVG: - syncer = RecRawStatSyncHrTimeAvg; - break; - default: - syncer = RecRawStatSyncCount; - break; - } + int id = ts::Metrics::getInstance().newMetric(the_name); - switch (persist) { - case TS_STAT_PERSISTENT: - RecRegisterRawStat(api_rsb, RECT_PLUGIN, the_name, (RecDataT)the_type, RECP_PERSISTENT, id, syncer); - break; - case TS_STAT_NON_PERSISTENT: - RecRegisterRawStat(api_rsb, RECT_PLUGIN, the_name, (RecDataT)the_type, RECP_NON_PERSISTENT, id, syncer); - break; - default: + if (id == ts::Metrics::NOT_FOUND) { return TS_ERROR; } @@ -7555,49 +7505,44 @@ void TSStatIntIncrement(int id, TSMgmtInt amount) { sdk_assert(sdk_sanity_check_stat_id(id) == TS_SUCCESS); - RecIncrRawStat(api_rsb, nullptr, id, amount); + ts::Metrics::getInstance().increment(id, amount); } void TSStatIntDecrement(int id, TSMgmtInt amount) { - RecDecrRawStat(api_rsb, nullptr, id, amount); + sdk_assert(sdk_sanity_check_stat_id(id) == TS_SUCCESS); + ts::Metrics::getInstance().decrement(id, amount); } TSMgmtInt TSStatIntGet(int id) { - TSMgmtInt value; - sdk_assert(sdk_sanity_check_stat_id(id) == TS_SUCCESS); - RecGetGlobalRawStatSum(api_rsb, id, &value); - return value; + return ts::Metrics::getInstance().get(id); } void TSStatIntSet(int id, TSMgmtInt value) { sdk_assert(sdk_sanity_check_stat_id(id) == TS_SUCCESS); - RecSetGlobalRawStatSum(api_rsb, id, value); + ts::Metrics::getInstance().set(id, value); } TSReturnCode TSStatFindName(const char *name, int *idp) { - int id; - sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)idp) == TS_SUCCESS); - if (RecGetRecordOrderAndId(name, nullptr, &id, true, true) != REC_ERR_OKAY) { - return TS_ERROR; - } + int id = ts::Metrics::getInstance().lookup(name); - if (RecGetGlobalRawStatPtr(api_rsb, id) == nullptr) { + if (id == ts::Metrics::NOT_FOUND) { return TS_ERROR; + } else { + *idp = id; + return TS_SUCCESS; } - - *idp = id; - return TS_SUCCESS; } /************************** Tracing API ****************************/ diff --git a/src/traffic_server/Makefile.inc b/src/traffic_server/Makefile.inc index 54469db224..e4ba7b9c0c 100644 --- a/src/traffic_server/Makefile.inc +++ b/src/traffic_server/Makefile.inc @@ -81,6 +81,7 @@ traffic_server_traffic_server_LDADD = \ $(top_builddir)/iocore/aio/libinkaio.a \ $(top_builddir)/src/tscore/libtscore.la \ $(top_builddir)/src/tscpp/util/libtscpputil.la \ + $(top_builddir)/src/api/libtsapi.a \ $(top_builddir)/proxy/libproxy.a \ $(top_builddir)/iocore/net/libinknet.a \ $(top_builddir)/src/records/librecords_p.a \