Skip to content

cryptomb: add queue size statistics #19180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggreenway merged 15 commits into from
Jan 4, 2022
Merged

cryptomb: add queue size statistics #19180

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5ca6d3c
Add cryptomb queue size statistics.
VillePihlava Nov 5, 2021
30768ff
Add missing newlines.
VillePihlava Dec 3, 2021
b2df330
Remove unnecessary -> void.
VillePihlava Dec 7, 2021
2e51af7
Change getQueueSizeCounters to queueSizeCounters.
VillePihlava Dec 7, 2021
e634b2e
Add comment.
VillePihlava Dec 9, 2021
1728354
Refactor setRsaKey and initialize BIGNUM pointers to nullptr.
VillePihlava Dec 9, 2021
3a86b6d
Change stats_prefix_ to local variable and remove unnecessary comments.
VillePihlava Dec 9, 2021
95d11b6
Refactor tests.
VillePihlava Dec 14, 2021
2f0528f
Fix format.
VillePihlava Dec 14, 2021
7d784fe
Change queue size counters to histogram.
VillePihlava Dec 15, 2021
024ab51
Change stats test to work with histogram.
VillePihlava Dec 15, 2021
c790431
Add callbacks back.
VillePihlava Dec 16, 2021
a6c3626
Add sub test classes and fix format.
VillePihlava Dec 16, 2021
a906fee
Change comment and fix format.
VillePihlava Dec 16, 2021
3fe1d40
Fix variable names.
VillePihlava Dec 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions contrib/cryptomb/private_key_providers/source/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ envoy_cc_library(
repository = "@envoy",
visibility = ["//visibility:public"],
deps = [
":cryptomb_stats_lib",
":ipp_crypto_wrapper_lib",
"//envoy/api:api_interface",
"//envoy/event:dispatcher_interface",
Expand All @@ -83,6 +84,21 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "cryptomb_stats_lib",
srcs = [
"cryptomb_stats.cc",
],
hdrs = [
"cryptomb_stats.h",
],
deps = [
"//envoy/stats:stats_interface",
"//source/common/stats:symbol_table_lib",
"//source/common/stats:utility_lib",
],
)

envoy_cc_contrib_extension(
name = "config",
srcs = ["config.cc"],
Expand Down
17 changes: 10 additions & 7 deletions contrib/cryptomb/private_key_providers/source/cryptomb_private_key_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,10 @@ ssl_private_key_result_t rsaPrivateKeyDecryptForTest(CryptoMbPrivateKeyConnectio
}

CryptoMbQueue::CryptoMbQueue(std::chrono::milliseconds poll_delay, enum KeyType type, int keysize,
IppCryptoSharedPtr ipp, Event::Dispatcher& d)
IppCryptoSharedPtr ipp, Event::Dispatcher& d, CryptoMbStats& stats)
: us_(std::chrono::duration_cast<std::chrono::microseconds>(poll_delay)), type_(type),
key_size_(keysize), ipp_(ipp),
timer_(d.createTimer([this]() -> void { processRequests(); })) {
key_size_(keysize), ipp_(ipp), timer_(d.createTimer([this]() -> void { processRequests(); })),

@jmarantz jmarantz Dec 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need the -> void

@VillePihlava VillePihlava Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed -> void from two places.

stats_(stats) {
request_queue_.reserve(MULTIBUFF_BATCH);
}

Expand All @@ -359,6 +359,8 @@ void CryptoMbQueue::addAndProcessEightRequests(CryptoMbContextSharedPtr mb_ctx)

void CryptoMbQueue::processRequests() {
if (type_ == KeyType::Rsa) {
// Increment correct queue size statistic.
stats_.getQueueSizeCounters()[request_queue_.size() - 1].get().inc();

@jmarantz jmarantz Dec 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/getQueueSizeCounters/queueSizeCounters/ per convention

@VillePihlava VillePihlava Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed after changing counters to histogram.

processRsaRequests();
}
request_queue_.clear();
Expand Down Expand Up @@ -486,7 +488,9 @@ CryptoMbPrivateKeyMethodProvider::CryptoMbPrivateKeyMethodProvider(
CryptoMbPrivateKeyMethodConfig& conf,
Server::Configuration::TransportSocketFactoryContext& factory_context, IppCryptoSharedPtr ipp)
: api_(factory_context.api()),
tls_(ThreadLocal::TypedSlot<ThreadLocalData>::makeUnique(factory_context.threadLocal())) {
tls_(ThreadLocal::TypedSlot<ThreadLocalData>::makeUnique(factory_context.threadLocal())),
stats_(factory_context.scope(), CryptoMbQueue::MULTIBUFF_BATCH, "cryptomb",
"rsa_queue_size_") {

@jmarantz jmarantz Dec 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this does not get called in response to requests, right? this happens at config-time?

If this does happen in response to requests then we should pre-symbolize the stat names and I can help you solve that without taking symbol-table locks. If this is called on config it's ok. But given it's not obvious how often this gets created, we should either comment on that assumption, or we should just go ahead and pre-symbolize all the names during config or (better yet) during startup.

@VillePihlava VillePihlava Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens at config-time, I added a comment about it.


if (!ipp->mbxIsCryptoMbApplicable(0)) {
throw EnvoyException("Multi-buffer CPU instructions not available.");
Expand Down Expand Up @@ -519,7 +523,6 @@ CryptoMbPrivateKeyMethodProvider::CryptoMbPrivateKeyMethodProvider(
method_->complete = privateKeyComplete;

RSA* rsa = EVP_PKEY_get0_RSA(pkey.get());

switch (RSA_bits(rsa)) {
case 1024:
key_size = 1024;
Expand Down Expand Up @@ -582,9 +585,9 @@ CryptoMbPrivateKeyMethodProvider::CryptoMbPrivateKeyMethodProvider(
enum KeyType key_type = key_type_;

// Create a single queue for every worker thread to avoid locking.
tls_->set([poll_delay, key_type, key_size, ipp](Event::Dispatcher& d) {
tls_->set([poll_delay, key_type, key_size, ipp, this](Event::Dispatcher& d) {
ENVOY_LOG(debug, "Created CryptoMb Queue for thread {}", d.name());
return std::make_shared<ThreadLocalData>(poll_delay, key_type, key_size, ipp, d);
return std::make_shared<ThreadLocalData>(poll_delay, key_type, key_size, ipp, d, stats_);
});
}

Expand Down
12 changes: 9 additions & 3 deletions contrib/cryptomb/private_key_providers/source/cryptomb_private_key_provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "source/common/common/c_smart_ptr.h"
#include "source/common/common/logger.h"

#include "contrib/cryptomb/private_key_providers/source/cryptomb_stats.h"
#include "contrib/cryptomb/private_key_providers/source/ipp_crypto.h"
#include "contrib/envoy/extensions/private_key_providers/cryptomb/v3alpha/cryptomb.pb.h"

Expand Down Expand Up @@ -93,7 +94,7 @@ class CryptoMbQueue : public Logger::Loggable<Logger::Id::connection> {
static constexpr uint32_t MULTIBUFF_BATCH = 8;

CryptoMbQueue(std::chrono::milliseconds poll_delay, enum KeyType type, int keysize,
IppCryptoSharedPtr ipp, Event::Dispatcher& d);
IppCryptoSharedPtr ipp, Event::Dispatcher& d, CryptoMbStats& stats);
void addAndProcessEightRequests(CryptoMbContextSharedPtr mb_ctx);

private:
Expand All @@ -120,6 +121,9 @@ class CryptoMbQueue : public Logger::Loggable<Logger::Id::connection> {

// Timer to trigger queue processing if eight requests are not received in time.
Event::TimerPtr timer_{};

// Reference to statistics.
CryptoMbStats& stats_;
};

// CryptoMbPrivateKeyConnection maintains the data needed by a given SSL
Expand Down Expand Up @@ -170,8 +174,8 @@ class CryptoMbPrivateKeyMethodProvider : public virtual Ssl::PrivateKeyMethodPro
// Thread local data containing a single queue per worker thread.
struct ThreadLocalData : public ThreadLocal::ThreadLocalObject {
ThreadLocalData(std::chrono::milliseconds poll_delay, enum KeyType type, int keysize,
IppCryptoSharedPtr ipp, Event::Dispatcher& d)
: queue_(poll_delay, type, keysize, ipp, d){};
IppCryptoSharedPtr ipp, Event::Dispatcher& d, CryptoMbStats& stats)
: queue_(poll_delay, type, keysize, ipp, d, stats){};
CryptoMbQueue queue_;
};

Expand All @@ -181,6 +185,8 @@ class CryptoMbPrivateKeyMethodProvider : public virtual Ssl::PrivateKeyMethodPro
enum KeyType key_type_;

ThreadLocal::TypedSlotPtr<ThreadLocalData> tls_;

CryptoMbStats stats_;
};

} // namespace CryptoMb
Expand Down
26 changes: 26 additions & 0 deletions contrib/cryptomb/private_key_providers/source/cryptomb_stats.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include "contrib/cryptomb/private_key_providers/source/cryptomb_stats.h"

#include "source/common/stats/utility.h"

#include "absl/strings/str_cat.h"

namespace Envoy {
namespace Extensions {
namespace PrivateKeyMethodProvider {
namespace CryptoMb {

CryptoMbStats::CryptoMbStats(Stats::Scope& scope, uint32_t max_queue_size,
absl::string_view stats_prefix,
absl::string_view queue_size_stat_prefix)
: stat_name_pool_(scope.symbolTable()), stats_prefix_(stat_name_pool_.add(stats_prefix)) {

@jmarantz jmarantz Dec 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stats_prefix_ is not referenced after the constructor so you can drop the member variable and make this a local.

@VillePihlava VillePihlava Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not exist anymore after the histogram change.

queue_size_counters_.reserve(max_queue_size);
for (uint32_t i = 1; i <= max_queue_size; i++) {
queue_size_counters_.push_back(Stats::Utility::counterFromStatNames(
scope, {stats_prefix_, stat_name_pool_.add(absl::StrCat(queue_size_stat_prefix, i))}));
}
}

} // namespace CryptoMb
} // namespace PrivateKeyMethodProvider
} // namespace Extensions
} // namespace Envoy
35 changes: 35 additions & 0 deletions contrib/cryptomb/private_key_providers/source/cryptomb_stats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#pragma once

#include <string>
#include <vector>

#include "envoy/stats/scope.h"

#include "source/common/stats/symbol_table_impl.h"

#include "absl/strings/string_view.h"

namespace Envoy {
namespace Extensions {
namespace PrivateKeyMethodProvider {
namespace CryptoMb {

using StatsCounterRef = std::reference_wrapper<Stats::Counter>;

class CryptoMbStats {
public:
CryptoMbStats(Stats::Scope& scope, uint32_t max_queue_size, absl::string_view stats_prefix,
absl::string_view queue_size_stat_prefix);
std::vector<StatsCounterRef>& getQueueSizeCounters() { return queue_size_counters_; }

private:
Stats::StatNamePool stat_name_pool_;
const Stats::StatName stats_prefix_;
// Vector for queue size statistics.
std::vector<StatsCounterRef> queue_size_counters_;
};

} // namespace CryptoMb
} // namespace PrivateKeyMethodProvider
} // namespace Extensions
} // namespace Envoy
Loading