From 6695db2005f0760dee30a1af05c5f189b78cc381 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Tue, 11 Oct 2016 17:22:54 -0700 Subject: [PATCH 01/10] Initial commit of partition id stats --- source/common/dynamo/dynamo_filter.cc | 28 +++ source/common/dynamo/dynamo_filter.h | 1 + source/common/dynamo/dynamo_request_parser.cc | 26 +++ source/common/dynamo/dynamo_request_parser.h | 12 ++ source/common/json/json_loader.cc | 16 ++ source/common/json/json_loader.h | 19 +- source/precompiled/precompiled.h | 1 + test/common/dynamo/dynamo_filter_test.cc | 188 ++++++++++++++++++ .../dynamo/dynamo_request_parser_test.cc | 57 ++++++ test/common/json/json_loader_test.cc | 17 ++ 10 files changed, 363 insertions(+), 2 deletions(-) diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index fa871d5aba104..c3ef955bc780c 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -57,6 +57,8 @@ void DynamoFilter::onEncodeComplete(const Buffer::Instance& data) { chargeBasicStats(status); + chargeTablePartitionIdStats(data); + if (Http::CodeUtility::is4xx(status)) { chargeFailureSpecificStats(data); } @@ -173,6 +175,7 @@ void DynamoFilter::chargeUnProcessedKeysStats(const Buffer::Instance& data) { // complete apart of the batch operation. Only the table names will be logged for errors. std::vector unprocessed_tables = Dynamo::RequestParser::parseBatchUnProcessedKeys(body); + for (const std::string& unprocessed_table : unprocessed_tables) { stats_.counter(fmt::format("{}error.{}.BatchFailureUnprocessedKeys", stat_prefix_, unprocessed_table)).inc(); @@ -208,4 +211,29 @@ void DynamoFilter::chargeFailureSpecificStats(const Buffer::Instance& data) { } } +void DynamoFilter::chargeTablePartitionIdStats(const Buffer::Instance& data) { + + if (table_descriptor_.table_name.empty()) { + return; + } + std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); + if (!body.empty()) { + try { + // Check if there is a partition id in the response + std::vector partitions = + Dynamo::RequestParser::parsePartitionIds(body); + for (const Dynamo::RequestParser::PartitionDescriptor& partition : partitions) { + // increment table partition stat stats_.counter + stats_.counter(fmt::format("{}table.{}.__partition_id={}", stat_prefix_, + table_descriptor_.table_name, partition.partition_id_)) + .add(partition.capacity_); + } + + } catch (const Json::Exception& jsonEx) { + // Body parsing failed. This should not happen, just put a stat for that. + stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); + } + } +} + } // Dynamo diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index 2decba7e01834..9a55eaa1c75fa 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -46,6 +46,7 @@ class DynamoFilter : public Http::StreamFilter { uint64_t status); void chargeFailureSpecificStats(const Buffer::Instance& data); void chargeUnProcessedKeysStats(const Buffer::Instance& data); + void chargeTablePartitionIdStats(const Buffer::Instance& data); Runtime::Loader& runtime_; std::string stat_prefix_; diff --git a/source/common/dynamo/dynamo_request_parser.cc b/source/common/dynamo/dynamo_request_parser.cc index 40b55f3c857de..23c557b6f1522 100644 --- a/source/common/dynamo/dynamo_request_parser.cc +++ b/source/common/dynamo/dynamo_request_parser.cc @@ -125,4 +125,30 @@ bool RequestParser::isBatchOperation(const std::string& operation) { BATCH_OPERATIONS.end(); } +std::vector +RequestParser::parsePartitionIds(const std::string& data) { + Json::StringLoader json(data); + std::vector partition_descriptors; + + if (json.hasObject("ConsumedCapacity")) { + Json::Object consumed_capacity = json.getObject("ConsumedCapacity"); + if (consumed_capacity.hasObject("Partitions")) { + Json::Object partitions = consumed_capacity.getObject("Partitions"); + + partitions.iterate( + [&partition_descriptors, &partitions](const std::string& key, const Json::Object&) { + // Capacity is a double and it is rounded up to the nearest integer. + // The stats for partition is incremented based on the capacity. + // Ex if Capacity == 2, the stats for key will be incremented by 2. + uint64_t capacity_integer = + static_cast(std::ceil(partitions.getDouble(key, 0.0))); + // Dynamo::RequestParser::PartitionDescriptor partition = {key, capacity_integer}; + partition_descriptors.emplace_back(key, capacity_integer); + return true; + }); + } + } + return partition_descriptors; +} + } // Dynamo \ No newline at end of file diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index 8d8fe9f4cadf7..a9945677a6391 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -18,6 +18,13 @@ class RequestParser { bool is_single_table; }; + struct PartitionDescriptor { + PartitionDescriptor(std::string partition, uint64_t capacity): partition_id_(partition), capacity_(capacity){} + std::string partition_id_; + // The capacity returned with a partition id is a real number. We round up the capacity. + uint64_t capacity_; + }; + /** * Parse operation out of x-amz-target header. * @return empty string if operation cannot be parsed. @@ -66,6 +73,11 @@ class RequestParser { */ static bool isBatchOperation(const std::string& operation); + /** + * @return empty set if there are no partition ids or a set or partition ids + */ + static std::vector parsePartitionIds(const std::string& data); + private: static const Http::LowerCaseString X_AMZ_TARGET; static const std::vector SINGLE_TABLE_OPERATIONS; diff --git a/source/common/json/json_loader.cc b/source/common/json/json_loader.cc index c5c153e1f25d1..ac108ce9f3862 100644 --- a/source/common/json/json_loader.cc +++ b/source/common/json/json_loader.cc @@ -136,6 +136,22 @@ std::vector Object::getStringArray(const std::string& name) const { return string_array; } +double Object::getDouble(const std::string& name) const { + json_t* real = json_object_get(json_, name.c_str()); + if (!real || !json_is_real(real)) { + throw Exception(fmt::format("key '{}' missing or not an double in '{}'", name, name_)); + } + + return json_real_value(real); +} + +double Object::getDouble(const std::string& name, const double& default_value) const { + if (!json_object_get(json_, name.c_str())) { + return default_value; + } else { + return getDouble(name); + } +} void Object::iterate(const ObjectCallback& callback) { const char* key; json_t* value; diff --git a/source/common/json/json_loader.h b/source/common/json/json_loader.h index aa10ad08b8ba7..6a56e0ea28583 100644 --- a/source/common/json/json_loader.h +++ b/source/common/json/json_loader.h @@ -81,14 +81,14 @@ class Object { /** * Get a string value by name. - * @param name suppies the key name. + * @param name supplies the key name. * @return std::string the value. */ std::string getString(const std::string& name) const; /** * Get a string value by name or return a default if name does not exist. - * @param name suppies the key name. + * @param name supplies the key name. * @param default_value supplies the value to return if name does not exist. * @return std::string the value. */ @@ -101,6 +101,21 @@ class Object { */ std::vector getStringArray(const std::string& name) const; + /** + * Get a double value by name. + * @param name supplies the key name. + * @return double the value + */ + double getDouble(const std::string& name) const; + + /** + * Get a double value by name. + * @param name supplies the key name. + * @param default_value supplies the value to return if name does not exist. + * @return double the value + */ + double getDouble(const std::string& name, const double& default_value) const; + /** * Iterate Object and call callback on key-value pairs */ diff --git a/source/precompiled/precompiled.h b/source/precompiled/precompiled.h index 9e205bd853559..3a4457f78482a 100644 --- a/source/precompiled/precompiled.h +++ b/source/precompiled/precompiled.h @@ -2,6 +2,7 @@ #include #include +#include #include #include #include diff --git a/test/common/dynamo/dynamo_filter_test.cc b/test/common/dynamo/dynamo_filter_test.cc index 309b8f285d4a7..dfc6ed88d5038 100644 --- a/test/common/dynamo/dynamo_filter_test.cc +++ b/test/common/dynamo/dynamo_filter_test.cc @@ -410,4 +410,192 @@ TEST_F(DynamoFilterTest, operatorPresentRuntimeDisabled) { EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_headers)); } +TEST_F(DynamoFilterTest, PartitionIdStats) { + setup(true); + + Http::HeaderMapImpl request_headers{{"x-amz-target", "version.GetItem"}}; + Buffer::OwnedImpl buffer; + std::string buffer_content = "{\"TableName\":\"locations\""; + buffer.add(buffer_content); + EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); + Buffer::OwnedImpl data; + data.add("}", 1); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(data, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, true)); + + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total_2xx")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total_200")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total")); + + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.GetItem.upstream_rq_time_200", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.GetItem.upstream_rq_time", _)); + + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_2xx")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); + + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_2xx", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_200", _)); + EXPECT_CALL(stats_, deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time", _)); + + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_1")).Times(1); + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_2")).Times(1); + +Http::HeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); + + Buffer::OwnedImpl empty_data; + Buffer::OwnedImpl response_data; + std::string response_content = R"EOF( + { + "ConsumedCapacity": { + "Partitions": { + "partition_1" : 0.5, + "partition_2" : 3.0 + } + } + } + )EOF"; + + response_data.add(response_content); + + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(1).WillRepeatedly(Return(&response_data)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); +} + +TEST_F(DynamoFilterTest, NoPartitionIdStatsForMultipleTables) { +setup(true); + +Http::HeaderMapImpl request_headers{{"x-amz-target", "version.BatchGetItem"}}; +Buffer::OwnedImpl buffer; +std::string buffer_content = R"EOF( +{ + "RequestItems": { + "table_1": { "test1" : "something" }, + "table_2": { "test2" : "something" } + } +} +)EOF"; +buffer.add(buffer_content); +EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); + +EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); +EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); +EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers)); + +EXPECT_CALL(stats_, counter("prefix.dynamodb.multiple_tables")); + +EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total")); +EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); +EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); + +EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", _)); +EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", _)); +EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", _)); + +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_1")).Times(0); +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_2")).Times(0); + +Http::HeaderMapImpl response_headers{{":status", "200"}}; +EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); + +Buffer::OwnedImpl empty_data; +Buffer::OwnedImpl response_data; +std::string response_content = R"EOF( + { + "ConsumedCapacity": { + "Partitions": { + "partition_1" : 0.5, + "partition_2" : 3.0 + } + } + } + )EOF"; + +response_data.add(response_content); + +EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(1).WillRepeatedly(Return(&response_data)); +EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); +} + +TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { +setup(true); + +Http::HeaderMapImpl request_headers{{"x-amz-target", "version.BatchGetItem"}}; +Buffer::OwnedImpl buffer; +std::string buffer_content = R"EOF( +{ + "RequestItems": { + "locations": { "test1" : "something" } + } +} +)EOF"; +buffer.add(buffer_content); +EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); + +EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); +EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); +EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers)); + +EXPECT_CALL(stats_, counter("prefix.dynamodb.multiple_tables")).Times(0); + +EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total")); +EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); +EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); + +EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", _)); +EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", _)); +EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", _)); + + +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_2xx")); +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); + +EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_2xx", _)); +EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_200", _)); +EXPECT_CALL(stats_, deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time", _)); + +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_1")).Times(1); +EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_2")).Times(1); + +Http::HeaderMapImpl response_headers{{":status", "200"}}; +EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); + +Buffer::OwnedImpl empty_data; +Buffer::OwnedImpl response_data; +std::string response_content = R"EOF( + { + "ConsumedCapacity": { + "Partitions": { + "partition_1" : 0.5, + "partition_2" : 3.0 + } + } + } + )EOF"; + +response_data.add(response_content); + +EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(2).WillRepeatedly(Return(&response_data)); +EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); +} + + } // Dynamo diff --git a/test/common/dynamo/dynamo_request_parser_test.cc b/test/common/dynamo/dynamo_request_parser_test.cc index 254b849b1e6c7..60efee44098a7 100644 --- a/test/common/dynamo/dynamo_request_parser_test.cc +++ b/test/common/dynamo/dynamo_request_parser_test.cc @@ -221,4 +221,61 @@ TEST(DynamoRequestParser, parseBatchUnProcessedKeys) { EXPECT_EQ(2u, unprocessed_tables.size()); } } + +TEST(DynamoRequestParser, parsePartitionIds) { + { + std::vector partitions = + RequestParser::parsePartitionIds("{}"); + EXPECT_EQ(0u, partitions.size()); + } + { + std::vector partitions = + RequestParser::parsePartitionIds("{\"ConsumedCapacity\":{}}"); + EXPECT_EQ(0u, partitions.size()); + } + { + std::vector partitions = + RequestParser::parsePartitionIds("{\"ConsumedCapacity\":{ \"Partitions\":{}}}"); + EXPECT_EQ(0u, partitions.size()); + } + { + std::string json = R"EOF( + { + "ConsumedCapacity": { + "Partitions": { + "partition_1" : 2.0, + "partition_2" : 3.0 + } + } + } + )EOF"; + std::vector partitions = + RequestParser::parsePartitionIds(json); + // EXPECT_TRUE(find(partitions.begin(), partitions.end(), "partition_1") != partitions.end()); + // EXPECT_TRUE(find(partitions.begin(), partitions.end(), "partition_2") != partitions.end()); + EXPECT_EQ(2u, partitions.size()); + } + { + std::string json = R"EOF( + { + "ConsumedCapacity": { + "Partitions": { + "partition_1" : 0.5, + "partition_2" : 3.0 + } + } + } + )EOF"; + std::vector partitions = + RequestParser::parsePartitionIds(json); + for (const RequestParser::PartitionDescriptor& partition : partitions) { + if (partition.partition_id_ == "partition_1") { + EXPECT_EQ(1u, partition.capacity_); + } else { + EXPECT_EQ(3u, partition.capacity_); + } + } + } +} + } // Dynamo \ No newline at end of file diff --git a/test/common/json/json_loader_test.cc b/test/common/json/json_loader_test.cc index f625801d724c9..2346bbe7d1260 100644 --- a/test/common/json/json_loader_test.cc +++ b/test/common/json/json_loader_test.cc @@ -129,4 +129,21 @@ TEST(JsonLoaderTest, Integer) { } } +TEST(JsonLoaderTest, Double) { + { + StringLoader json("{\"value1\": 10.5, \"value2\": -12.3}"); + EXPECT_EQ(10.5, json.getDouble("value1")); + EXPECT_EQ(-12.3, json.getDouble("value2")); + } + { + StringLoader json("{\"foo\": 13.22}"); + EXPECT_EQ(13.22, json.getDouble("foo", 0)); + EXPECT_EQ(0, json.getDouble("bar", 0)); + } + { + StringLoader json("{\"foo\": \"bar\"}"); + EXPECT_THROW(json.getDouble("foo"), Exception); + } +} + } // Json From 46732a443181074bdbb0876723a356a44b48051c Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 13 Oct 2016 10:31:49 -0700 Subject: [PATCH 02/10] Partition Id stats --- source/common/dynamo/dynamo_filter.cc | 34 ++- source/common/dynamo/dynamo_filter.h | 8 +- source/common/dynamo/dynamo_request_parser.cc | 6 +- source/common/dynamo/dynamo_request_parser.h | 7 +- source/common/json/json_loader.cc | 1 + source/common/json/json_loader.h | 4 +- test/common/dynamo/dynamo_filter_test.cc | 200 ++++++++++++------ .../dynamo/dynamo_request_parser_test.cc | 10 +- 8 files changed, 175 insertions(+), 95 deletions(-) diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index c3ef955bc780c..a23dfcf0c393a 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -175,7 +175,6 @@ void DynamoFilter::chargeUnProcessedKeysStats(const Buffer::Instance& data) { // complete apart of the batch operation. Only the table names will be logged for errors. std::vector unprocessed_tables = Dynamo::RequestParser::parseBatchUnProcessedKeys(body); - for (const std::string& unprocessed_table : unprocessed_tables) { stats_.counter(fmt::format("{}error.{}.BatchFailureUnprocessedKeys", stat_prefix_, unprocessed_table)).inc(); @@ -212,23 +211,20 @@ void DynamoFilter::chargeFailureSpecificStats(const Buffer::Instance& data) { } void DynamoFilter::chargeTablePartitionIdStats(const Buffer::Instance& data) { - - if (table_descriptor_.table_name.empty()) { + // Only log partition stats for single table operations. + if (table_descriptor_.table_name.empty() || operation_.empty()) { return; } std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); if (!body.empty()) { try { - // Check if there is a partition id in the response std::vector partitions = - Dynamo::RequestParser::parsePartitionIds(body); + Dynamo::RequestParser::parsePartitions(body); for (const Dynamo::RequestParser::PartitionDescriptor& partition : partitions) { - // increment table partition stat stats_.counter - stats_.counter(fmt::format("{}table.{}.__partition_id={}", stat_prefix_, - table_descriptor_.table_name, partition.partition_id_)) - .add(partition.capacity_); + std::string stats_string = DynamoFilter::buildPartitionStatString( + stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); + stats_.counter(stats_string).add(partition.capacity_); } - } catch (const Json::Exception& jsonEx) { // Body parsing failed. This should not happen, just put a stat for that. stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); @@ -236,4 +232,22 @@ void DynamoFilter::chargeTablePartitionIdStats(const Buffer::Instance& data) { } } +std::string DynamoFilter::buildPartitionStatString(const std::string& stat_prefix, + const std::string& table_name, + const std::string& operation, + const std::string& partition_id) { + std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); + // Use only the last 7 characters from the partition id. + std::string stats_partition_postfix = + fmt::format(".Capacity.{}.__partition_id={}", operation, + partition_id.substr(partition_id.size() - 7, partition_id.size())); + // Calculate how many characters are available for the table prefix. + size_t remaining_size = MAX_NAME_SIZE - stats_partition_postfix.size() - 1; + // Truncate the table prefix if the curent string is too large to fit. + if (stats_table_prefix.size() > remaining_size) { + stats_table_prefix = stats_table_prefix.substr(0, remaining_size); + } + return fmt::format("{}{}", stats_table_prefix, stats_partition_postfix); +} + } // Dynamo diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index 9a55eaa1c75fa..d4a3b8731d3e3 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -37,6 +37,11 @@ class DynamoFilter : public Http::StreamFilter { encoder_callbacks_ = &callbacks; } + static std::string buildPartitionStatString(const std::string& stat_prefix, + const std::string& table_name, + const std::string& operation, + const std::string& partition_id); + private: void onDecodeComplete(const Buffer::Instance& data); void onEncodeComplete(const Buffer::Instance& data); @@ -48,10 +53,11 @@ class DynamoFilter : public Http::StreamFilter { void chargeUnProcessedKeysStats(const Buffer::Instance& data); void chargeTablePartitionIdStats(const Buffer::Instance& data); + static const size_t MAX_NAME_SIZE = 127; + Runtime::Loader& runtime_; std::string stat_prefix_; Stats::Store& stats_; - bool enabled_{}; std::string operation_{}; RequestParser::TableDescriptor table_descriptor_{"", true}; diff --git a/source/common/dynamo/dynamo_request_parser.cc b/source/common/dynamo/dynamo_request_parser.cc index 23c557b6f1522..c294165bcacff 100644 --- a/source/common/dynamo/dynamo_request_parser.cc +++ b/source/common/dynamo/dynamo_request_parser.cc @@ -126,7 +126,7 @@ bool RequestParser::isBatchOperation(const std::string& operation) { } std::vector -RequestParser::parsePartitionIds(const std::string& data) { +RequestParser::parsePartitions(const std::string& data) { Json::StringLoader json(data); std::vector partition_descriptors; @@ -138,11 +138,9 @@ RequestParser::parsePartitionIds(const std::string& data) { partitions.iterate( [&partition_descriptors, &partitions](const std::string& key, const Json::Object&) { // Capacity is a double and it is rounded up to the nearest integer. - // The stats for partition is incremented based on the capacity. - // Ex if Capacity == 2, the stats for key will be incremented by 2. + // The partition stat will be incremented by the capacity value. uint64_t capacity_integer = static_cast(std::ceil(partitions.getDouble(key, 0.0))); - // Dynamo::RequestParser::PartitionDescriptor partition = {key, capacity_integer}; partition_descriptors.emplace_back(key, capacity_integer); return true; }); diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index a9945677a6391..8cb5cc3932138 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -19,7 +19,8 @@ class RequestParser { }; struct PartitionDescriptor { - PartitionDescriptor(std::string partition, uint64_t capacity): partition_id_(partition), capacity_(capacity){} + PartitionDescriptor(std::string partition, uint64_t capacity) + : partition_id_(partition), capacity_(capacity) {} std::string partition_id_; // The capacity returned with a partition id is a real number. We round up the capacity. uint64_t capacity_; @@ -74,9 +75,9 @@ class RequestParser { static bool isBatchOperation(const std::string& operation); /** - * @return empty set if there are no partition ids or a set or partition ids + * @return empty set if there are no partition ids or a set of partition ids */ - static std::vector parsePartitionIds(const std::string& data); + static std::vector parsePartitions(const std::string& data); private: static const Http::LowerCaseString X_AMZ_TARGET; diff --git a/source/common/json/json_loader.cc b/source/common/json/json_loader.cc index ac108ce9f3862..f83c6f1728592 100644 --- a/source/common/json/json_loader.cc +++ b/source/common/json/json_loader.cc @@ -152,6 +152,7 @@ double Object::getDouble(const std::string& name, const double& default_value) c return getDouble(name); } } + void Object::iterate(const ObjectCallback& callback) { const char* key; json_t* value; diff --git a/source/common/json/json_loader.h b/source/common/json/json_loader.h index 6a56e0ea28583..3a9f3e590ec8a 100644 --- a/source/common/json/json_loader.h +++ b/source/common/json/json_loader.h @@ -104,7 +104,7 @@ class Object { /** * Get a double value by name. * @param name supplies the key name. - * @return double the value + * @return double the value. */ double getDouble(const std::string& name) const; @@ -112,7 +112,7 @@ class Object { * Get a double value by name. * @param name supplies the key name. * @param default_value supplies the value to return if name does not exist. - * @return double the value + * @return double the value. */ double getDouble(const std::string& name, const double& default_value) const; diff --git a/test/common/dynamo/dynamo_filter_test.cc b/test/common/dynamo/dynamo_filter_test.cc index dfc6ed88d5038..e8274a8cda793 100644 --- a/test/common/dynamo/dynamo_filter_test.cc +++ b/test/common/dynamo/dynamo_filter_test.cc @@ -446,10 +446,14 @@ TEST_F(DynamoFilterTest, PartitionIdStats) { deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_200", _)); EXPECT_CALL(stats_, deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time", _)); - EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_1")).Times(1); - EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_2")).Times(1); + EXPECT_CALL(stats_, + counter("prefix.dynamodb.table.locations.Capacity.GetItem.__partition_id=ition_1")) + .Times(1); + EXPECT_CALL(stats_, + counter("prefix.dynamodb.table.locations.Capacity.GetItem.__partition_id=ition_2")) + .Times(1); -Http::HeaderMapImpl response_headers{{":status", "200"}}; + Http::HeaderMapImpl response_headers{{":status", "200"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); Buffer::OwnedImpl empty_data; @@ -472,11 +476,11 @@ Http::HeaderMapImpl response_headers{{":status", "200"}}; } TEST_F(DynamoFilterTest, NoPartitionIdStatsForMultipleTables) { -setup(true); + setup(true); -Http::HeaderMapImpl request_headers{{"x-amz-target", "version.BatchGetItem"}}; -Buffer::OwnedImpl buffer; -std::string buffer_content = R"EOF( + Http::HeaderMapImpl request_headers{{"x-amz-target", "version.BatchGetItem"}}; + Buffer::OwnedImpl buffer; + std::string buffer_content = R"EOF( { "RequestItems": { "table_1": { "test1" : "something" }, @@ -484,35 +488,41 @@ std::string buffer_content = R"EOF( } } )EOF"; -buffer.add(buffer_content); -EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); + buffer.add(buffer_content); + EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); -EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); -EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); -EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers)); -EXPECT_CALL(stats_, counter("prefix.dynamodb.multiple_tables")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.multiple_tables")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); -EXPECT_CALL(stats_, deliverTimingToSinks( - "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", _)); -EXPECT_CALL(stats_, deliverTimingToSinks( - "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", _)); -EXPECT_CALL(stats_, - deliverTimingToSinks("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", _)); + EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", _)); + EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", _)); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_1")).Times(0); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_2")).Times(0); + EXPECT_CALL( + stats_, + counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_1")) + .Times(0); + EXPECT_CALL( + stats_, + counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_2")) + .Times(0); -Http::HeaderMapImpl response_headers{{":status", "200"}}; -EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); + Http::HeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); -Buffer::OwnedImpl empty_data; -Buffer::OwnedImpl response_data; -std::string response_content = R"EOF( + Buffer::OwnedImpl empty_data; + Buffer::OwnedImpl response_data; + std::string response_content = R"EOF( { "ConsumedCapacity": { "Partitions": { @@ -523,64 +533,69 @@ std::string response_content = R"EOF( } )EOF"; -response_data.add(response_content); + response_data.add(response_content); -EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(1).WillRepeatedly(Return(&response_data)); -EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(1).WillRepeatedly(Return(&response_data)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); } TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { -setup(true); + setup(true); -Http::HeaderMapImpl request_headers{{"x-amz-target", "version.BatchGetItem"}}; -Buffer::OwnedImpl buffer; -std::string buffer_content = R"EOF( + Http::HeaderMapImpl request_headers{{"x-amz-target", "version.BatchGetItem"}}; + Buffer::OwnedImpl buffer; + std::string buffer_content = R"EOF( { "RequestItems": { "locations": { "test1" : "something" } } } )EOF"; -buffer.add(buffer_content); -EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); - -EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); -EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); -EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers)); + buffer.add(buffer_content); + EXPECT_CALL(decoder_callbacks_, decodingBuffer()).WillRepeatedly(Return(&buffer)); -EXPECT_CALL(stats_, counter("prefix.dynamodb.multiple_tables")).Times(0); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(buffer, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers)); -EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.multiple_tables")).Times(0); -EXPECT_CALL(stats_, deliverTimingToSinks( - "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", _)); -EXPECT_CALL(stats_, deliverTimingToSinks( - "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", _)); -EXPECT_CALL(stats_, - deliverTimingToSinks("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", _)); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); + EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", _)); + EXPECT_CALL(stats_, deliverTimingToSinks( + "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", _)); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_2xx")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_2xx")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); + EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); -EXPECT_CALL(stats_, - deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_2xx", _)); -EXPECT_CALL(stats_, - deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_200", _)); -EXPECT_CALL(stats_, deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_2xx", _)); + EXPECT_CALL(stats_, + deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time_200", _)); + EXPECT_CALL(stats_, deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time", _)); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_1")).Times(1); -EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.__partition_id=partition_2")).Times(1); + EXPECT_CALL( + stats_, + counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_1")) + .Times(1); + EXPECT_CALL( + stats_, + counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_2")) + .Times(1); -Http::HeaderMapImpl response_headers{{":status", "200"}}; -EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); + Http::HeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); -Buffer::OwnedImpl empty_data; -Buffer::OwnedImpl response_data; -std::string response_content = R"EOF( + Buffer::OwnedImpl empty_data; + Buffer::OwnedImpl response_data; + std::string response_content = R"EOF( { "ConsumedCapacity": { "Partitions": { @@ -591,11 +606,56 @@ std::string response_content = R"EOF( } )EOF"; -response_data.add(response_content); + response_data.add(response_content); -EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(2).WillRepeatedly(Return(&response_data)); -EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(2).WillRepeatedly(Return(&response_data)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); } +TEST_F(DynamoFilterTest, PartitionIdStatString) { + const size_t MAX_NAME_SIZE = 127; + { + std::string stat_prefix = "stat.prefix."; + std::string table_name = "locations"; + std::string operation = "GetItem"; + std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; + std::string partition_stat_string = + DynamoFilter::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = + "stat.prefix.table.locations.Capacity.GetItem.__partition_id=c5883ca"; + EXPECT_EQ(expected_stat_string, partition_stat_string); + EXPECT_TRUE(partition_stat_string.size() < MAX_NAME_SIZE); + } + + { + std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; + std::string table_name = "locations-sandbox-partition-test-iad-mytest-really-long-name"; + std::string operation = "GetItem"; + std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; + + std::string partition_stat_string = + DynamoFilter::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" + "partition-test-iad-mytest-re.Capacity.GetItem.__partition_" + "id=c5883ca"; + EXPECT_EQ(expected_stat_string, partition_stat_string); + EXPECT_TRUE(partition_stat_string.size() < MAX_NAME_SIZE); + } + { + std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; + std::string table_name = "locations-sandbox-partition-test-iad-mytest-re"; + std::string operation = "GetItem"; + std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; + + std::string partition_stat_string = + DynamoFilter::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" + "partition-test-iad-mytest-re.Capacity.GetItem.__partition_" + "id=c5883ca"; + + EXPECT_EQ(expected_stat_string, partition_stat_string); + EXPECT_TRUE(partition_stat_string.size() < MAX_NAME_SIZE); + } +} } // Dynamo diff --git a/test/common/dynamo/dynamo_request_parser_test.cc b/test/common/dynamo/dynamo_request_parser_test.cc index 60efee44098a7..a7a6cee056d23 100644 --- a/test/common/dynamo/dynamo_request_parser_test.cc +++ b/test/common/dynamo/dynamo_request_parser_test.cc @@ -225,17 +225,17 @@ TEST(DynamoRequestParser, parseBatchUnProcessedKeys) { TEST(DynamoRequestParser, parsePartitionIds) { { std::vector partitions = - RequestParser::parsePartitionIds("{}"); + RequestParser::parsePartitions("{}"); EXPECT_EQ(0u, partitions.size()); } { std::vector partitions = - RequestParser::parsePartitionIds("{\"ConsumedCapacity\":{}}"); + RequestParser::parsePartitions("{\"ConsumedCapacity\":{}}"); EXPECT_EQ(0u, partitions.size()); } { std::vector partitions = - RequestParser::parsePartitionIds("{\"ConsumedCapacity\":{ \"Partitions\":{}}}"); + RequestParser::parsePartitions("{\"ConsumedCapacity\":{ \"Partitions\":{}}}"); EXPECT_EQ(0u, partitions.size()); } { @@ -250,7 +250,7 @@ TEST(DynamoRequestParser, parsePartitionIds) { } )EOF"; std::vector partitions = - RequestParser::parsePartitionIds(json); + RequestParser::parsePartitions(json); // EXPECT_TRUE(find(partitions.begin(), partitions.end(), "partition_1") != partitions.end()); // EXPECT_TRUE(find(partitions.begin(), partitions.end(), "partition_2") != partitions.end()); EXPECT_EQ(2u, partitions.size()); @@ -267,7 +267,7 @@ TEST(DynamoRequestParser, parsePartitionIds) { } )EOF"; std::vector partitions = - RequestParser::parsePartitionIds(json); + RequestParser::parsePartitions(json); for (const RequestParser::PartitionDescriptor& partition : partitions) { if (partition.partition_id_ == "partition_1") { EXPECT_EQ(1u, partition.capacity_); From 3eca2297953c147dac91ebf33997f88b57ae0bb7 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 13 Oct 2016 10:42:34 -0700 Subject: [PATCH 03/10] Update comments --- source/common/dynamo/dynamo_filter.cc | 2 +- source/common/dynamo/dynamo_request_parser.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index a23dfcf0c393a..321aaf16ea252 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -243,7 +243,7 @@ std::string DynamoFilter::buildPartitionStatString(const std::string& stat_prefi partition_id.substr(partition_id.size() - 7, partition_id.size())); // Calculate how many characters are available for the table prefix. size_t remaining_size = MAX_NAME_SIZE - stats_partition_postfix.size() - 1; - // Truncate the table prefix if the curent string is too large to fit. + // Truncate the table prefix if the curent string is too large. if (stats_table_prefix.size() > remaining_size) { stats_table_prefix = stats_table_prefix.substr(0, remaining_size); } diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index 8cb5cc3932138..19a475e095e2d 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -22,7 +22,6 @@ class RequestParser { PartitionDescriptor(std::string partition, uint64_t capacity) : partition_id_(partition), capacity_(capacity) {} std::string partition_id_; - // The capacity returned with a partition id is a real number. We round up the capacity. uint64_t capacity_; }; @@ -75,6 +74,7 @@ class RequestParser { static bool isBatchOperation(const std::string& operation); /** + * Parse the Partition ids and the capacity from the response body. * @return empty set if there are no partition ids or a set of partition ids */ static std::vector parsePartitions(const std::string& data); From 9ed557d7f8699e2bc736554cd43a00581e6640bf Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 13 Oct 2016 13:04:34 -0700 Subject: [PATCH 04/10] Address Roman's pr comments --- .../http_filters/dynamodb_filter.rst | 12 ++++ docs/intro/arch_overview/dynamo.rst | 2 +- source/common/CMakeLists.txt | 1 + source/common/dynamo/dynamo_filter.cc | 23 +------- source/common/dynamo/dynamo_filter.h | 7 --- source/common/dynamo/dynamo_request_parser.h | 2 +- source/common/dynamo/dynamo_utility.cc | 25 ++++++++ source/common/dynamo/dynamo_utility.h | 13 +++++ source/common/stats/stats_impl.cc | 2 +- test/CMakeLists.txt | 1 + test/common/dynamo/dynamo_filter_test.cc | 58 ++----------------- test/common/dynamo/dynamo_utility_test.cc | 53 +++++++++++++++++ 12 files changed, 117 insertions(+), 82 deletions(-) create mode 100644 source/common/dynamo/dynamo_utility.cc create mode 100644 source/common/dynamo/dynamo_utility.h create mode 100644 test/common/dynamo/dynamo_utility_test.cc diff --git a/docs/configuration/http_filters/dynamodb_filter.rst b/docs/configuration/http_filters/dynamodb_filter.rst index 99cbe54863b34..08ba0eb4dbd5a 100644 --- a/docs/configuration/http_filters/dynamodb_filter.rst +++ b/docs/configuration/http_filters/dynamodb_filter.rst @@ -54,6 +54,18 @@ in all operations from the batch. upstream_rq_total_xxx, Counter, Total number of requests on table per response code (503/2xx/etc) upstream_rq_time_xxx, Timer, Time spent on table per response code (400/3xx/etc) +*Disclaimer: Please note that this is a pre-release Amazon DynamoDB feature that is not yet widely available.* +Per partition and operation stats can be found in the *http..dynamodb.table..* +namespace. For Batch operations, Envoy tracks per partition and operation stats in the case only if it is the same +table used in all operations from the batch. + + .. csv-table:: + :header: Name, Type, Description + :widths: 1, 1, 2 + + capacity..__partition_id=, Counter, Total number of + capacity for on table for a given + Additional detailed stats: * For 4xx responses and partial batch operation failures, the total number of failures for a given diff --git a/docs/intro/arch_overview/dynamo.rst b/docs/intro/arch_overview/dynamo.rst index ed072acdd52d8..d757fe5aa42df 100644 --- a/docs/intro/arch_overview/dynamo.rst +++ b/docs/intro/arch_overview/dynamo.rst @@ -6,7 +6,7 @@ DynamoDB Envoy supports an HTTP level DynamoDB sniffing filter with the following features: * DynamoDB API request/response parser. -* DynamoDB per operation/per table statistics. +* DynamoDB per operation/per table/per partition and operation statistics. * Failure type statistics for 4xx responses, parsed from response JSON, e.g., ProvisionedThroughputExceededException. * Batch operation partial failure statistics. diff --git a/source/common/CMakeLists.txt b/source/common/CMakeLists.txt index f79ab09cb1043..3bc7775129601 100644 --- a/source/common/CMakeLists.txt +++ b/source/common/CMakeLists.txt @@ -31,6 +31,7 @@ add_library( common/version.cc dynamo/dynamo_filter.cc dynamo/dynamo_request_parser.cc + dynamo/dynamo_utility.cc event/dispatcher_impl.cc event/event_impl_base.cc event/file_event_impl.cc diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index 321aaf16ea252..4684f2f890906 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -1,4 +1,5 @@ #include "dynamo_filter.h" +#include "dynamo_utility.h" #include "common/buffer/buffer_impl.h" #include "common/dynamo/dynamo_request_parser.h" @@ -201,7 +202,7 @@ void DynamoFilter::chargeFailureSpecificStats(const Buffer::Instance& data) { error_type)).inc(); } } - } catch (const Json::Exception& jsonEx) { + } catch (const Json::Exception&) { // Body parsing failed. This should not happen, just put a stat for that. stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); } @@ -221,7 +222,7 @@ void DynamoFilter::chargeTablePartitionIdStats(const Buffer::Instance& data) { std::vector partitions = Dynamo::RequestParser::parsePartitions(body); for (const Dynamo::RequestParser::PartitionDescriptor& partition : partitions) { - std::string stats_string = DynamoFilter::buildPartitionStatString( + std::string stats_string = Utility::buildPartitionStatString( stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); stats_.counter(stats_string).add(partition.capacity_); } @@ -232,22 +233,4 @@ void DynamoFilter::chargeTablePartitionIdStats(const Buffer::Instance& data) { } } -std::string DynamoFilter::buildPartitionStatString(const std::string& stat_prefix, - const std::string& table_name, - const std::string& operation, - const std::string& partition_id) { - std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); - // Use only the last 7 characters from the partition id. - std::string stats_partition_postfix = - fmt::format(".Capacity.{}.__partition_id={}", operation, - partition_id.substr(partition_id.size() - 7, partition_id.size())); - // Calculate how many characters are available for the table prefix. - size_t remaining_size = MAX_NAME_SIZE - stats_partition_postfix.size() - 1; - // Truncate the table prefix if the curent string is too large. - if (stats_table_prefix.size() > remaining_size) { - stats_table_prefix = stats_table_prefix.substr(0, remaining_size); - } - return fmt::format("{}{}", stats_table_prefix, stats_partition_postfix); -} - } // Dynamo diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index d4a3b8731d3e3..a28d34dfb07b6 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -37,11 +37,6 @@ class DynamoFilter : public Http::StreamFilter { encoder_callbacks_ = &callbacks; } - static std::string buildPartitionStatString(const std::string& stat_prefix, - const std::string& table_name, - const std::string& operation, - const std::string& partition_id); - private: void onDecodeComplete(const Buffer::Instance& data); void onEncodeComplete(const Buffer::Instance& data); @@ -53,8 +48,6 @@ class DynamoFilter : public Http::StreamFilter { void chargeUnProcessedKeysStats(const Buffer::Instance& data); void chargeTablePartitionIdStats(const Buffer::Instance& data); - static const size_t MAX_NAME_SIZE = 127; - Runtime::Loader& runtime_; std::string stat_prefix_; Stats::Store& stats_; diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index 19a475e095e2d..7a65622d343fd 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -19,7 +19,7 @@ class RequestParser { }; struct PartitionDescriptor { - PartitionDescriptor(std::string partition, uint64_t capacity) + PartitionDescriptor(const std::string& partition, uint64_t capacity) : partition_id_(partition), capacity_(capacity) {} std::string partition_id_; uint64_t capacity_; diff --git a/source/common/dynamo/dynamo_utility.cc b/source/common/dynamo/dynamo_utility.cc new file mode 100644 index 0000000000000..2b2fd73ab49be --- /dev/null +++ b/source/common/dynamo/dynamo_utility.cc @@ -0,0 +1,25 @@ +#include "dynamo_utility.h" + +#include "common/stats/stats_impl.h" + +namespace Dynamo { + +std::string Utility::buildPartitionStatString(const std::string& stat_prefix, + const std::string& table_name, + const std::string& operation, + const std::string& partition_id) { + std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); + // Use only the last 7 characters from the partition id. + std::string stats_partition_postfix = + fmt::format(".capacity.{}.__partition_id={}", operation, + partition_id.substr(partition_id.size() - 7, partition_id.size())); + // Calculate how many characters are available for the table prefix. + size_t remaining_size = Stats::RawStatData::MAX_NAME_SIZE - stats_partition_postfix.size(); + // Truncate the table prefix if the curent string is too large. + if (stats_table_prefix.size() > remaining_size) { + stats_table_prefix = stats_table_prefix.substr(0, remaining_size); + } + return fmt::format("{}{}", stats_table_prefix, stats_partition_postfix); +} + +} // Dynamo diff --git a/source/common/dynamo/dynamo_utility.h b/source/common/dynamo/dynamo_utility.h new file mode 100644 index 0000000000000..64ba71dff40cc --- /dev/null +++ b/source/common/dynamo/dynamo_utility.h @@ -0,0 +1,13 @@ +#pragma once + +namespace Dynamo { + +class Utility { +public: + static std::string buildPartitionStatString(const std::string& stat_prefix, + const std::string& table_name, + const std::string& operation, + const std::string& partition_id); +}; + +} // Dynamo diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index ab2cae6e578ca..902b696ec7a60 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -16,7 +16,7 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { void RawStatData::initialize(const std::string& name) { ASSERT(!initialized()); - ASSERT(name.size() < MAX_NAME_SIZE); + ASSERT(name.size() <= MAX_NAME_SIZE); strncpy(name_, name.substr(0, MAX_NAME_SIZE).c_str(), MAX_NAME_SIZE + 1); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b543ed1376a10..e15cbd642d464 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -58,6 +58,7 @@ add_executable(envoy-test common/filter/tcp_proxy_test.cc common/dynamo/dynamo_filter_test.cc common/dynamo/dynamo_request_parser_test.cc + common/dynamo/dynamo_utility_test.cc common/json/json_loader_test.cc common/mongo/bson_impl_test.cc common/mongo/codec_impl_test.cc diff --git a/test/common/dynamo/dynamo_filter_test.cc b/test/common/dynamo/dynamo_filter_test.cc index e8274a8cda793..cb9b9d702ff16 100644 --- a/test/common/dynamo/dynamo_filter_test.cc +++ b/test/common/dynamo/dynamo_filter_test.cc @@ -447,10 +447,10 @@ TEST_F(DynamoFilterTest, PartitionIdStats) { EXPECT_CALL(stats_, deliverTimingToSinks("prefix.dynamodb.table.locations.upstream_rq_time", _)); EXPECT_CALL(stats_, - counter("prefix.dynamodb.table.locations.Capacity.GetItem.__partition_id=ition_1")) + counter("prefix.dynamodb.table.locations.capacity.GetItem.__partition_id=ition_1")) .Times(1); EXPECT_CALL(stats_, - counter("prefix.dynamodb.table.locations.Capacity.GetItem.__partition_id=ition_2")) + counter("prefix.dynamodb.table.locations.capacity.GetItem.__partition_id=ition_2")) .Times(1); Http::HeaderMapImpl response_headers{{":status", "200"}}; @@ -510,11 +510,11 @@ TEST_F(DynamoFilterTest, NoPartitionIdStatsForMultipleTables) { EXPECT_CALL( stats_, - counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_1")) + counter("prefix.dynamodb.table.locations.capacity.BatchGetItem.__partition_id=ition_1")) .Times(0); EXPECT_CALL( stats_, - counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_2")) + counter("prefix.dynamodb.table.locations.capacity.BatchGetItem.__partition_id=ition_2")) .Times(0); Http::HeaderMapImpl response_headers{{":status", "200"}}; @@ -583,11 +583,11 @@ TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { EXPECT_CALL( stats_, - counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_1")) + counter("prefix.dynamodb.table.locations.capacity.BatchGetItem.__partition_id=ition_1")) .Times(1); EXPECT_CALL( stats_, - counter("prefix.dynamodb.table.locations.Capacity.BatchGetItem.__partition_id=ition_2")) + counter("prefix.dynamodb.table.locations.capacity.BatchGetItem.__partition_id=ition_2")) .Times(1); Http::HeaderMapImpl response_headers{{":status", "200"}}; @@ -612,50 +612,4 @@ TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); } -TEST_F(DynamoFilterTest, PartitionIdStatString) { - const size_t MAX_NAME_SIZE = 127; - { - std::string stat_prefix = "stat.prefix."; - std::string table_name = "locations"; - std::string operation = "GetItem"; - std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; - std::string partition_stat_string = - DynamoFilter::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); - std::string expected_stat_string = - "stat.prefix.table.locations.Capacity.GetItem.__partition_id=c5883ca"; - EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() < MAX_NAME_SIZE); - } - - { - std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; - std::string table_name = "locations-sandbox-partition-test-iad-mytest-really-long-name"; - std::string operation = "GetItem"; - std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; - - std::string partition_stat_string = - DynamoFilter::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); - std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" - "partition-test-iad-mytest-re.Capacity.GetItem.__partition_" - "id=c5883ca"; - EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() < MAX_NAME_SIZE); - } - { - std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; - std::string table_name = "locations-sandbox-partition-test-iad-mytest-re"; - std::string operation = "GetItem"; - std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; - - std::string partition_stat_string = - DynamoFilter::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); - std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" - "partition-test-iad-mytest-re.Capacity.GetItem.__partition_" - "id=c5883ca"; - - EXPECT_EQ(expected_stat_string, partition_stat_string); - EXPECT_TRUE(partition_stat_string.size() < MAX_NAME_SIZE); - } -} - } // Dynamo diff --git a/test/common/dynamo/dynamo_utility_test.cc b/test/common/dynamo/dynamo_utility_test.cc new file mode 100644 index 0000000000000..bea71125e5d02 --- /dev/null +++ b/test/common/dynamo/dynamo_utility_test.cc @@ -0,0 +1,53 @@ +#include "common/dynamo/dynamo_utility.h" +#include "common/stats/stats_impl.h" + +using testing::_; + +namespace Dynamo { + +TEST(DynamoUtility, PartitionIdStatString) { + { + std::string stat_prefix = "stat.prefix."; + std::string table_name = "locations"; + std::string operation = "GetItem"; + std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; + std::string partition_stat_string = + Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = + "stat.prefix.table.locations.capacity.GetItem.__partition_id=c5883ca"; + EXPECT_EQ(expected_stat_string, partition_stat_string); + EXPECT_TRUE(partition_stat_string.size() <= Stats::RawStatData::MAX_NAME_SIZE); + } + + { + std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; + std::string table_name = "locations-sandbox-partition-test-iad-mytest-really-long-name"; + std::string operation = "GetItem"; + std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; + + std::string partition_stat_string = + Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" + "partition-test-iad-mytest-rea.capacity.GetItem.__partition_" + "id=c5883ca"; + EXPECT_EQ(expected_stat_string, partition_stat_string); + EXPECT_TRUE(partition_stat_string.size() == Stats::RawStatData::MAX_NAME_SIZE); + } + { + std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; + std::string table_name = "locations-sandbox-partition-test-iad-mytest-rea"; + std::string operation = "GetItem"; + std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; + + std::string partition_stat_string = + Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" + "partition-test-iad-mytest-rea.capacity.GetItem.__partition_" + "id=c5883ca"; + + EXPECT_EQ(expected_stat_string, partition_stat_string); + EXPECT_TRUE(partition_stat_string.size() == Stats::RawStatData::MAX_NAME_SIZE); + } +} + +} // Dynamo From 6532da82414003698fb2a8e46fc5c3affc51d424 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 13 Oct 2016 13:06:19 -0700 Subject: [PATCH 05/10] Add newline --- source/common/dynamo/dynamo_filter.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index a28d34dfb07b6..9a55eaa1c75fa 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -51,6 +51,7 @@ class DynamoFilter : public Http::StreamFilter { Runtime::Loader& runtime_; std::string stat_prefix_; Stats::Store& stats_; + bool enabled_{}; std::string operation_{}; RequestParser::TableDescriptor table_descriptor_{"", true}; From 1a3197f975ee60ae0f361fc4505dd8aae93faa07 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 13 Oct 2016 16:09:39 -0700 Subject: [PATCH 06/10] Expand comments --- .../http_filters/dynamodb_filter.rst | 7 ++-- source/common/dynamo/dynamo_filter.cc | 37 ++++++++----------- source/common/dynamo/dynamo_filter.h | 6 +-- source/common/dynamo/dynamo_request_parser.cc | 30 +++++++-------- source/common/dynamo/dynamo_request_parser.h | 7 +++- source/common/dynamo/dynamo_utility.cc | 8 ++-- source/common/dynamo/dynamo_utility.h | 10 +++++ source/common/json/json_loader.cc | 4 +- source/common/json/json_loader.h | 2 +- test/common/dynamo/dynamo_filter_test.cc | 2 +- .../dynamo/dynamo_request_parser_test.cc | 18 +-------- 11 files changed, 61 insertions(+), 70 deletions(-) diff --git a/docs/configuration/http_filters/dynamodb_filter.rst b/docs/configuration/http_filters/dynamodb_filter.rst index 08ba0eb4dbd5a..ab16d93cfe7de 100644 --- a/docs/configuration/http_filters/dynamodb_filter.rst +++ b/docs/configuration/http_filters/dynamodb_filter.rst @@ -56,15 +56,14 @@ in all operations from the batch. *Disclaimer: Please note that this is a pre-release Amazon DynamoDB feature that is not yet widely available.* Per partition and operation stats can be found in the *http..dynamodb.table..* -namespace. For Batch operations, Envoy tracks per partition and operation stats in the case only if it is the same -table used in all operations from the batch. +namespace. For batch operations, Envoy tracks per partition and operation stats only if it is the same +table used in all operations. .. csv-table:: :header: Name, Type, Description :widths: 1, 1, 2 - capacity..__partition_id=, Counter, Total number of - capacity for on table for a given + capacity..__partition_id=, Counter, Total number of capacity for on table for a given Additional detailed stats: diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index 4684f2f890906..63906f5930f9c 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -13,7 +13,7 @@ namespace Dynamo { Http::FilterHeadersStatus DynamoFilter::decodeHeaders(Http::HeaderMap& headers, bool) { if (enabled_) { start_decode_ = std::chrono::system_clock::now(); - operation_ = Dynamo::RequestParser::parseOperation(headers); + operation_ = RequestParser::parseOperation(headers); } return Http::FilterHeadersStatus::Continue; @@ -44,7 +44,7 @@ void DynamoFilter::onDecodeComplete(const Buffer::Instance& data) { std::string body = buildBody(decoder_callbacks_->decodingBuffer(), data); if (!body.empty()) { try { - table_descriptor_ = Dynamo::RequestParser::parseTable(operation_, body); + table_descriptor_ = RequestParser::parseTable(operation_, body); } catch (const Json::Exception& jsonEx) { // Body parsing failed. This should not happen, just put a stat for that. stats_.counter(fmt::format("{}invalid_req_body", stat_prefix_)).inc(); @@ -57,17 +57,17 @@ void DynamoFilter::onEncodeComplete(const Buffer::Instance& data) { uint64_t status = Http::Utility::getResponseStatus(*response_headers_); chargeBasicStats(status); - - chargeTablePartitionIdStats(data); + std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); + chargeTablePartitionIdStats(body); if (Http::CodeUtility::is4xx(status)) { - chargeFailureSpecificStats(data); + chargeFailureSpecificStats(body); } // Batch Operations will always return status 200 for a partial or full success. Check // unprocessed keys to determine partial success. // http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Programming.Errors.html#Programming.Errors.BatchOperations - if (Dynamo::RequestParser::isBatchOperation(operation_)) { - chargeUnProcessedKeysStats(data); + if (RequestParser::isBatchOperation(operation_)) { + chargeUnProcessedKeysStats(body); } } } @@ -168,8 +168,7 @@ void DynamoFilter::chargeStatsPerEntity(const std::string& entity, const std::st latency); } -void DynamoFilter::chargeUnProcessedKeysStats(const Buffer::Instance& data) { - std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); +void DynamoFilter::chargeUnProcessedKeysStats(const std::string& body) { if (!body.empty()) { try { // The unprocessed keys block contains a list of tables and keys for that table that did not @@ -180,19 +179,17 @@ void DynamoFilter::chargeUnProcessedKeysStats(const Buffer::Instance& data) { stats_.counter(fmt::format("{}error.{}.BatchFailureUnprocessedKeys", stat_prefix_, unprocessed_table)).inc(); } - } catch (const Json::Exception& jsonEx) { + } catch (const Json::Exception&) { // Body parsing failed. This should not happen, just put a stat for that. stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); } } } -void DynamoFilter::chargeFailureSpecificStats(const Buffer::Instance& data) { - std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); - +void DynamoFilter::chargeFailureSpecificStats(const std::string& body) { if (!body.empty()) { try { - std::string error_type = Dynamo::RequestParser::parseErrorType(body); + std::string error_type = RequestParser::parseErrorType(body); if (!error_type.empty()) { if (table_descriptor_.table_name.empty()) { @@ -211,22 +208,20 @@ void DynamoFilter::chargeFailureSpecificStats(const Buffer::Instance& data) { } } -void DynamoFilter::chargeTablePartitionIdStats(const Buffer::Instance& data) { - // Only log partition stats for single table operations. +void DynamoFilter::chargeTablePartitionIdStats(const std::string& body) { if (table_descriptor_.table_name.empty() || operation_.empty()) { return; } - std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); if (!body.empty()) { try { - std::vector partitions = - Dynamo::RequestParser::parsePartitions(body); - for (const Dynamo::RequestParser::PartitionDescriptor& partition : partitions) { + std::vector partitions = + RequestParser::parsePartitions(body); + for (const RequestParser::PartitionDescriptor& partition : partitions) { std::string stats_string = Utility::buildPartitionStatString( stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); stats_.counter(stats_string).add(partition.capacity_); } - } catch (const Json::Exception& jsonEx) { + } catch (const Json::Exception&) { // Body parsing failed. This should not happen, just put a stat for that. stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); } diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index 9a55eaa1c75fa..0af95b2fca705 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -44,9 +44,9 @@ class DynamoFilter : public Http::StreamFilter { void chargeBasicStats(uint64_t status); void chargeStatsPerEntity(const std::string& entity, const std::string& entity_type, uint64_t status); - void chargeFailureSpecificStats(const Buffer::Instance& data); - void chargeUnProcessedKeysStats(const Buffer::Instance& data); - void chargeTablePartitionIdStats(const Buffer::Instance& data); + void chargeFailureSpecificStats(const std::string& body); + void chargeUnProcessedKeysStats(const std::string& body); + void chargeTablePartitionIdStats(const std::string& body); Runtime::Loader& runtime_; std::string stat_prefix_; diff --git a/source/common/dynamo/dynamo_request_parser.cc b/source/common/dynamo/dynamo_request_parser.cc index c294165bcacff..b8943dd60990d 100644 --- a/source/common/dynamo/dynamo_request_parser.cc +++ b/source/common/dynamo/dynamo_request_parser.cc @@ -130,22 +130,20 @@ RequestParser::parsePartitions(const std::string& data) { Json::StringLoader json(data); std::vector partition_descriptors; - if (json.hasObject("ConsumedCapacity")) { - Json::Object consumed_capacity = json.getObject("ConsumedCapacity"); - if (consumed_capacity.hasObject("Partitions")) { - Json::Object partitions = consumed_capacity.getObject("Partitions"); - - partitions.iterate( - [&partition_descriptors, &partitions](const std::string& key, const Json::Object&) { - // Capacity is a double and it is rounded up to the nearest integer. - // The partition stat will be incremented by the capacity value. - uint64_t capacity_integer = - static_cast(std::ceil(partitions.getDouble(key, 0.0))); - partition_descriptors.emplace_back(key, capacity_integer); - return true; - }); - } - } + Json::Object consumed_capacity = json.getObject("ConsumedCapacity", true); + Json::Object partitions = consumed_capacity.getObject("Partitions", true); + + partitions.iterate([&partition_descriptors, &partitions](const std::string& key, + const Json::Object&) { + // For a given partition id, the amount of capacity used is returned in the body as a double. + // A stat will be created to track the capacity consumed for the operation, table and partition. + // Stats counter only increments by whole numbers, capacity is round up to the nearest integer + // to account for this. + uint64_t capacity_integer = static_cast(std::ceil(partitions.getDouble(key, 0.0))); + partition_descriptors.emplace_back(key, capacity_integer); + return true; + }); + return partition_descriptors; } diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index 7a65622d343fd..06f812cec58b5 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -74,8 +74,11 @@ class RequestParser { static bool isBatchOperation(const std::string& operation); /** - * Parse the Partition ids and the capacity from the response body. - * @return empty set if there are no partition ids or a set of partition ids + * Parse the Partition ids and the consumed capacity from the body. + * @return empty set if there is no partition data or a set of partition data containing + * the partition id as a string and the capacity consumed as an integer. + * + * @throw Json::Exception if data is not in valid Json format. */ static std::vector parsePartitions(const std::string& data); diff --git a/source/common/dynamo/dynamo_utility.cc b/source/common/dynamo/dynamo_utility.cc index 2b2fd73ab49be..aed628de938eb 100644 --- a/source/common/dynamo/dynamo_utility.cc +++ b/source/common/dynamo/dynamo_utility.cc @@ -8,14 +8,16 @@ std::string Utility::buildPartitionStatString(const std::string& stat_prefix, const std::string& table_name, const std::string& operation, const std::string& partition_id) { - std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); - // Use only the last 7 characters from the partition id. + // Use the last 7 characters of the partition id. std::string stats_partition_postfix = fmt::format(".capacity.{}.__partition_id={}", operation, partition_id.substr(partition_id.size() - 7, partition_id.size())); + // Calculate how many characters are available for the table prefix. size_t remaining_size = Stats::RawStatData::MAX_NAME_SIZE - stats_partition_postfix.size(); - // Truncate the table prefix if the curent string is too large. + + std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); + // Truncate the table prefix if the current string is too large. if (stats_table_prefix.size() > remaining_size) { stats_table_prefix = stats_table_prefix.substr(0, remaining_size); } diff --git a/source/common/dynamo/dynamo_utility.h b/source/common/dynamo/dynamo_utility.h index 64ba71dff40cc..d4788cedc4863 100644 --- a/source/common/dynamo/dynamo_utility.h +++ b/source/common/dynamo/dynamo_utility.h @@ -4,6 +4,16 @@ namespace Dynamo { class Utility { public: + + /* + * Creates the partition id stats string. + * The stats format is "table..capacity..__partition_id=". + * Partition ids and dynamodb table names can be long. To satisfy the string length, + * we truncate in two ways: + * 1. We only take the last 7 characters of the partition id. + * 2. If the stats string with is longer than the stats MAX_NAME_SIZE, we will truncate the table name to + * fit the size requirements. + */ static std::string buildPartitionStatString(const std::string& stat_prefix, const std::string& table_name, const std::string& operation, diff --git a/source/common/json/json_loader.cc b/source/common/json/json_loader.cc index f83c6f1728592..cb7ae9fe8bb73 100644 --- a/source/common/json/json_loader.cc +++ b/source/common/json/json_loader.cc @@ -139,13 +139,13 @@ std::vector Object::getStringArray(const std::string& name) const { double Object::getDouble(const std::string& name) const { json_t* real = json_object_get(json_, name.c_str()); if (!real || !json_is_real(real)) { - throw Exception(fmt::format("key '{}' missing or not an double in '{}'", name, name_)); + throw Exception(fmt::format("key '{}' missing or not a double in '{}'", name, name_)); } return json_real_value(real); } -double Object::getDouble(const std::string& name, const double& default_value) const { +double Object::getDouble(const std::string& name, double default_value) const { if (!json_object_get(json_, name.c_str())) { return default_value; } else { diff --git a/source/common/json/json_loader.h b/source/common/json/json_loader.h index 3a9f3e590ec8a..95d435f2aa757 100644 --- a/source/common/json/json_loader.h +++ b/source/common/json/json_loader.h @@ -114,7 +114,7 @@ class Object { * @param default_value supplies the value to return if name does not exist. * @return double the value. */ - double getDouble(const std::string& name, const double& default_value) const; + double getDouble(const std::string& name, double default_value) const; /** * Iterate Object and call callback on key-value pairs diff --git a/test/common/dynamo/dynamo_filter_test.cc b/test/common/dynamo/dynamo_filter_test.cc index cb9b9d702ff16..f22e0459bb366 100644 --- a/test/common/dynamo/dynamo_filter_test.cc +++ b/test/common/dynamo/dynamo_filter_test.cc @@ -608,7 +608,7 @@ TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { response_data.add(response_content); - EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(2).WillRepeatedly(Return(&response_data)); + EXPECT_CALL(encoder_callbacks_, encodingBuffer()).Times(1).WillRepeatedly(Return(&response_data)); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); } diff --git a/test/common/dynamo/dynamo_request_parser_test.cc b/test/common/dynamo/dynamo_request_parser_test.cc index a7a6cee056d23..b821dea6f0ab0 100644 --- a/test/common/dynamo/dynamo_request_parser_test.cc +++ b/test/common/dynamo/dynamo_request_parser_test.cc @@ -238,23 +238,6 @@ TEST(DynamoRequestParser, parsePartitionIds) { RequestParser::parsePartitions("{\"ConsumedCapacity\":{ \"Partitions\":{}}}"); EXPECT_EQ(0u, partitions.size()); } - { - std::string json = R"EOF( - { - "ConsumedCapacity": { - "Partitions": { - "partition_1" : 2.0, - "partition_2" : 3.0 - } - } - } - )EOF"; - std::vector partitions = - RequestParser::parsePartitions(json); - // EXPECT_TRUE(find(partitions.begin(), partitions.end(), "partition_1") != partitions.end()); - // EXPECT_TRUE(find(partitions.begin(), partitions.end(), "partition_2") != partitions.end()); - EXPECT_EQ(2u, partitions.size()); - } { std::string json = R"EOF( { @@ -275,6 +258,7 @@ TEST(DynamoRequestParser, parsePartitionIds) { EXPECT_EQ(3u, partition.capacity_); } } + EXPECT_EQ(2u, partitions.size()); } } From ae1e2e7cebdf95dfeec0e8788591a9f31a69637b Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 13 Oct 2016 16:17:27 -0700 Subject: [PATCH 07/10] fix format --- source/common/dynamo/dynamo_utility.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/dynamo/dynamo_utility.h b/source/common/dynamo/dynamo_utility.h index d4788cedc4863..b0c159cfb2c7a 100644 --- a/source/common/dynamo/dynamo_utility.h +++ b/source/common/dynamo/dynamo_utility.h @@ -4,14 +4,15 @@ namespace Dynamo { class Utility { public: - /* * Creates the partition id stats string. - * The stats format is "table..capacity..__partition_id=". + * The stats format is + * "table..capacity..__partition_id=". * Partition ids and dynamodb table names can be long. To satisfy the string length, * we truncate in two ways: * 1. We only take the last 7 characters of the partition id. - * 2. If the stats string with is longer than the stats MAX_NAME_SIZE, we will truncate the table name to + * 2. If the stats string with is longer than the stats MAX_NAME_SIZE, we will + * truncate the table name to * fit the size requirements. */ static std::string buildPartitionStatString(const std::string& stat_prefix, From 5887f6977735041b7bc343b9463c5ff90ff4a2b7 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Mon, 17 Oct 2016 11:34:57 -0700 Subject: [PATCH 08/10] Refactor json parsing. --- source/common/dynamo/dynamo_filter.cc | 107 +++++++--------- source/common/dynamo/dynamo_filter.h | 7 +- source/common/dynamo/dynamo_request_parser.cc | 74 +++++------ source/common/dynamo/dynamo_request_parser.h | 9 +- .../dynamo/dynamo_request_parser_test.cc | 117 +++++++++--------- 5 files changed, 142 insertions(+), 172 deletions(-) diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index 63906f5930f9c..c2dbc16f91b97 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -44,7 +44,8 @@ void DynamoFilter::onDecodeComplete(const Buffer::Instance& data) { std::string body = buildBody(decoder_callbacks_->decodingBuffer(), data); if (!body.empty()) { try { - table_descriptor_ = RequestParser::parseTable(operation_, body); + Json::StringLoader json_body(body); + table_descriptor_ = RequestParser::parseTable(operation_, json_body); } catch (const Json::Exception& jsonEx) { // Body parsing failed. This should not happen, just put a stat for that. stats_.counter(fmt::format("{}invalid_req_body", stat_prefix_)).inc(); @@ -53,21 +54,30 @@ void DynamoFilter::onDecodeComplete(const Buffer::Instance& data) { } void DynamoFilter::onEncodeComplete(const Buffer::Instance& data) { - if (response_headers_) { - uint64_t status = Http::Utility::getResponseStatus(*response_headers_); + if (!response_headers_) { + return; + } - chargeBasicStats(status); - std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); - chargeTablePartitionIdStats(body); + uint64_t status = Http::Utility::getResponseStatus(*response_headers_); + chargeBasicStats(status); + std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); + if (!body.empty()) { + try { + Json::StringLoader json_body(body); + chargeTablePartitionIdStats(json_body); - if (Http::CodeUtility::is4xx(status)) { - chargeFailureSpecificStats(body); - } - // Batch Operations will always return status 200 for a partial or full success. Check - // unprocessed keys to determine partial success. - // http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Programming.Errors.html#Programming.Errors.BatchOperations - if (RequestParser::isBatchOperation(operation_)) { - chargeUnProcessedKeysStats(body); + if (Http::CodeUtility::is4xx(status)) { + chargeFailureSpecificStats(json_body); + } + // Batch Operations will always return status 200 for a partial or full success. Check + // unprocessed keys to determine partial success. + // http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Programming.Errors.html#Programming.Errors.BatchOperations + if (RequestParser::isBatchOperation(operation_)) { + chargeUnProcessedKeysStats(json_body); + } + } catch (const Json::Exception&) { + // Body parsing failed. This should not happen, just put a stat for that. + stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); } } } @@ -168,63 +178,42 @@ void DynamoFilter::chargeStatsPerEntity(const std::string& entity, const std::st latency); } -void DynamoFilter::chargeUnProcessedKeysStats(const std::string& body) { - if (!body.empty()) { - try { - // The unprocessed keys block contains a list of tables and keys for that table that did not - // complete apart of the batch operation. Only the table names will be logged for errors. - std::vector unprocessed_tables = - Dynamo::RequestParser::parseBatchUnProcessedKeys(body); - for (const std::string& unprocessed_table : unprocessed_tables) { - stats_.counter(fmt::format("{}error.{}.BatchFailureUnprocessedKeys", stat_prefix_, - unprocessed_table)).inc(); - } - } catch (const Json::Exception&) { - // Body parsing failed. This should not happen, just put a stat for that. - stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); - } +void DynamoFilter::chargeUnProcessedKeysStats(const Json::Object& json_body) { + // The unprocessed keys block contains a list of tables and keys for that table that did not + // complete apart of the batch operation. Only the table names will be logged for errors. + std::vector unprocessed_tables = RequestParser::parseBatchUnProcessedKeys(json_body); + for (const std::string& unprocessed_table : unprocessed_tables) { + stats_.counter(fmt::format("{}error.{}.BatchFailureUnprocessedKeys", stat_prefix_, + unprocessed_table)).inc(); } } -void DynamoFilter::chargeFailureSpecificStats(const std::string& body) { - if (!body.empty()) { - try { - std::string error_type = RequestParser::parseErrorType(body); - - if (!error_type.empty()) { - if (table_descriptor_.table_name.empty()) { - stats_.counter(fmt::format("{}error.no_table.{}", stat_prefix_, error_type)).inc(); - } else { - stats_.counter(fmt::format("{}error.{}.{}", stat_prefix_, table_descriptor_.table_name, - error_type)).inc(); - } - } - } catch (const Json::Exception&) { - // Body parsing failed. This should not happen, just put a stat for that. - stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); +void DynamoFilter::chargeFailureSpecificStats(const Json::Object& json_body) { + std::string error_type = RequestParser::parseErrorType(json_body); + + if (!error_type.empty()) { + if (table_descriptor_.table_name.empty()) { + stats_.counter(fmt::format("{}error.no_table.{}", stat_prefix_, error_type)).inc(); + } else { + stats_.counter(fmt::format("{}error.{}.{}", stat_prefix_, table_descriptor_.table_name, + error_type)).inc(); } } else { stats_.counter(fmt::format("{}empty_response_body", stat_prefix_)).inc(); } } -void DynamoFilter::chargeTablePartitionIdStats(const std::string& body) { +void DynamoFilter::chargeTablePartitionIdStats(const Json::Object& json_body) { if (table_descriptor_.table_name.empty() || operation_.empty()) { return; } - if (!body.empty()) { - try { - std::vector partitions = - RequestParser::parsePartitions(body); - for (const RequestParser::PartitionDescriptor& partition : partitions) { - std::string stats_string = Utility::buildPartitionStatString( - stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); - stats_.counter(stats_string).add(partition.capacity_); - } - } catch (const Json::Exception&) { - // Body parsing failed. This should not happen, just put a stat for that. - stats_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); - } + + std::vector partitions = + RequestParser::parsePartitions(json_body); + for (const RequestParser::PartitionDescriptor& partition : partitions) { + std::string stats_string = Utility::buildPartitionStatString( + stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); + stats_.counter(stats_string).add(partition.capacity_); } } diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index 0af95b2fca705..99448b8312929 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -2,6 +2,7 @@ #include "dynamo_request_parser.h" +#include "common/json/json_loader.h" #include "envoy/http/filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/stats.h" @@ -44,9 +45,9 @@ class DynamoFilter : public Http::StreamFilter { void chargeBasicStats(uint64_t status); void chargeStatsPerEntity(const std::string& entity, const std::string& entity_type, uint64_t status); - void chargeFailureSpecificStats(const std::string& body); - void chargeUnProcessedKeysStats(const std::string& body); - void chargeTablePartitionIdStats(const std::string& body); + void chargeFailureSpecificStats(const Json::Object& json_body); + void chargeUnProcessedKeysStats(const Json::Object& json_body); + void chargeTablePartitionIdStats(const Json::Object& json_body); Runtime::Loader& runtime_; std::string stat_prefix_; diff --git a/source/common/dynamo/dynamo_request_parser.cc b/source/common/dynamo/dynamo_request_parser.cc index b8943dd60990d..0c744fec54f04 100644 --- a/source/common/dynamo/dynamo_request_parser.cc +++ b/source/common/dynamo/dynamo_request_parser.cc @@ -1,7 +1,6 @@ #include "dynamo_request_parser.h" #include "common/common/utility.h" -#include "common/json/json_loader.h" namespace Dynamo { @@ -60,60 +59,47 @@ std::string RequestParser::parseOperation(const Http::HeaderMap& headerMap) { } RequestParser::TableDescriptor RequestParser::parseTable(const std::string& operation, - const std::string& data) { + const Json::Object& json_data) { TableDescriptor table{"", true}; // Simple operations on a single table, have "TableName" explicitly specified. if (find(SINGLE_TABLE_OPERATIONS.begin(), SINGLE_TABLE_OPERATIONS.end(), operation) != SINGLE_TABLE_OPERATIONS.end()) { - Json::StringLoader json(data); - if (json.hasObject("TableName")) { - table.table_name = json.getString("TableName"); - } + table.table_name = json_data.getString("TableName", ""); } else if (find(BATCH_OPERATIONS.begin(), BATCH_OPERATIONS.end(), operation) != BATCH_OPERATIONS.end()) { - Json::StringLoader json(data); - if (json.hasObject("RequestItems")) { - Json::Object tables = json.getObject("RequestItems"); - tables.iterate([&table](const std::string& key, const Json::Object&) { - if (table.table_name.empty()) { - table.table_name = key; - } else { - if (table.table_name != key) { - table.table_name = ""; - table.is_single_table = false; - return false; - } + Json::Object tables = json_data.getObject("RequestItems", true); + tables.iterate([&table](const std::string& key, const Json::Object&) { + if (table.table_name.empty()) { + table.table_name = key; + } else { + if (table.table_name != key) { + table.table_name = ""; + table.is_single_table = false; + return false; } - - return true; - }); - } + } + return true; + }); } return table; } -std::vector RequestParser::parseBatchUnProcessedKeys(const std::string& data) { +std::vector RequestParser::parseBatchUnProcessedKeys(const Json::Object& json_data) { std::vector unprocessed_tables; - Json::StringLoader json(data); - if (json.hasObject("UnprocessedKeys")) { - Json::Object tables = json.getObject("UnprocessedKeys"); - tables.iterate([&unprocessed_tables](const std::string& key, const Json::Object&) { - unprocessed_tables.emplace_back(key); - return true; - }); - } + Json::Object tables = json_data.getObject("UnprocessedKeys", true); + tables.iterate([&unprocessed_tables](const std::string& key, const Json::Object&) { + unprocessed_tables.emplace_back(key); + return true; + }); + return unprocessed_tables; } -std::string RequestParser::parseErrorType(const std::string& data) { - Json::StringLoader json(data); - - if (json.hasObject("__type")) { - std::string error_type = json.getString("__type"); - for (const std::string& supported_error_type : SUPPORTED_ERROR_TYPES) { - if (StringUtil::endsWith(error_type, supported_error_type)) { - return supported_error_type; - } +std::string RequestParser::parseErrorType(const Json::Object& json_data) { + std::string error_type = json_data.getString("__type", ""); + for (const std::string& supported_error_type : SUPPORTED_ERROR_TYPES) { + if (StringUtil::endsWith(error_type, supported_error_type)) { + return supported_error_type; } } @@ -126,13 +112,11 @@ bool RequestParser::isBatchOperation(const std::string& operation) { } std::vector -RequestParser::parsePartitions(const std::string& data) { - Json::StringLoader json(data); +RequestParser::parsePartitions(const Json::Object& json_data) { std::vector partition_descriptors; - Json::Object consumed_capacity = json.getObject("ConsumedCapacity", true); - Json::Object partitions = consumed_capacity.getObject("Partitions", true); - + Json::Object partitions = + json_data.getObject("ConsumedCapacity", true).getObject("Partitions", true); partitions.iterate([&partition_descriptors, &partitions](const std::string& key, const Json::Object&) { // For a given partition id, the amount of capacity used is returned in the body as a double. diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index 06f812cec58b5..9c78a840d3073 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -1,5 +1,6 @@ #pragma once +#include "common/json/json_loader.h" #include "envoy/http/header_map.h" namespace Dynamo { @@ -47,7 +48,7 @@ class RequestParser { * * @throw Json::Exception if data is not in valid Json format. */ - static TableDescriptor parseTable(const std::string& operation, const std::string& data); + static TableDescriptor parseTable(const std::string& operation, const Json::Object& json_data); /** * Parse error details which might be provided for a given response code. @@ -59,14 +60,14 @@ class RequestParser { * * @throw Json::Exception if data is not in valid Json format. */ - static std::string parseErrorType(const std::string& data); + static std::string parseErrorType(const Json::Object& json_data); /** * Parse unprocessed keys for batch operation results. * @return empty set if there are no unprocessed keys or a set of table names that did not get * processed in the batch operation. */ - static std::vector parseBatchUnProcessedKeys(const std::string& data); + static std::vector parseBatchUnProcessedKeys(const Json::Object& json_data); /** * @return true if the operation is in the set of supported BATCH_OPERATIONS @@ -80,7 +81,7 @@ class RequestParser { * * @throw Json::Exception if data is not in valid Json format. */ - static std::vector parsePartitions(const std::string& data); + static std::vector parsePartitions(const Json::Object& json_data); private: static const Http::LowerCaseString X_AMZ_TARGET; diff --git a/test/common/dynamo/dynamo_request_parser_test.cc b/test/common/dynamo/dynamo_request_parser_test.cc index b821dea6f0ab0..b7bfd75855553 100644 --- a/test/common/dynamo/dynamo_request_parser_test.cc +++ b/test/common/dynamo/dynamo_request_parser_test.cc @@ -35,7 +35,7 @@ TEST(DynamoRequestParser, parseTableNameSingleOperation) { "PutItem", "UpdateItem", "DeleteItem"}; { - std::string json = R"EOF( + std::string json_string = R"EOF( { "TableName": "Pets", "Key": { @@ -44,72 +44,48 @@ TEST(DynamoRequestParser, parseTableNameSingleOperation) { } } )EOF"; + Json::StringLoader json_data(json_string); // Supported operation for (const std::string& operation : supported_single_operations) { - EXPECT_EQ("Pets", RequestParser::parseTable(operation, json).table_name); + EXPECT_EQ("Pets", RequestParser::parseTable(operation, json_data).table_name); } // Not supported operation - EXPECT_EQ("", RequestParser::parseTable("NotSupportedOperation", json).table_name); + EXPECT_EQ("", RequestParser::parseTable("NotSupportedOperation", json_data).table_name); } { - std::string json = "{\"TableName\":\"Pets\"}"; - EXPECT_EQ("Pets", RequestParser::parseTable("GetItem", json).table_name); - } - - // Validate invalid json cases - { - std::string json = R"EOF( - { - "TableName": "Pets", - "Key": { - "AnimalType": "S": "Dog"}, - "Name": {"S": "Fido"} - } - } - )EOF"; - - EXPECT_THROW(RequestParser::parseTable("GetItem", json), Json::Exception); + Json::StringLoader json_data("{\"TableName\":\"Pets\"}"); + EXPECT_EQ("Pets", RequestParser::parseTable("GetItem", json_data).table_name); } } TEST(DynamoRequestParser, parseErrorType) { - { EXPECT_THROW(RequestParser::parseErrorType("{test"), Json::Exception); } + { EXPECT_THROW(RequestParser::parseErrorType(Json::StringLoader("{test")), Json::Exception); } { EXPECT_EQ("ResourceNotFoundException", - RequestParser::parseErrorType( - "{\"__type\":\"com.amazonaws.dynamodb.v20120810#ResourceNotFoundException\"}")); + RequestParser::parseErrorType(Json::StringLoader( + "{\"__type\":\"com.amazonaws.dynamodb.v20120810#ResourceNotFoundException\"}"))); } { EXPECT_EQ("ResourceNotFoundException", - RequestParser::parseErrorType( + RequestParser::parseErrorType(Json::StringLoader( "{\"__type\":\"com.amazonaws.dynamodb.v20120810#ResourceNotFoundException\"," - "\"message\":\"Requested resource not found: Table: tablename not found\"}")); + "\"message\":\"Requested resource not found: Table: tablename not found\"}"))); } - { EXPECT_EQ("", RequestParser::parseErrorType("{\"__type\":\"UnKnownError\"}")); } - { - std::string json = R"EOF( - { - "not_important": "test", - "Key": { - "AnimalType": "S": "Dog"}, - "Name": {"S": "Fido"} - } - } - )EOF"; - EXPECT_THROW(RequestParser::parseErrorType(json), Json::Exception); + EXPECT_EQ("", + RequestParser::parseErrorType(Json::StringLoader("{\"__type\":\"UnKnownError\"}"))); } } TEST(DynamoRequestParser, parseTableNameBatchOperation) { { - std::string json = R"EOF( + std::string json_string = R"EOF( { "RequestItems": { "table_1": { "test1" : "something" }, @@ -117,13 +93,15 @@ TEST(DynamoRequestParser, parseTableNameBatchOperation) { } } )EOF"; - RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", json); + Json::StringLoader json_data(json_string); + + RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", json_data); EXPECT_EQ("", table.table_name); EXPECT_FALSE(table.is_single_table); } { - std::string json = R"EOF( + std::string json_string = R"EOF( { "RequestItems": { "table_2": { "test1" : "something" }, @@ -131,13 +109,15 @@ TEST(DynamoRequestParser, parseTableNameBatchOperation) { } } )EOF"; - RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", json); + Json::StringLoader json_data(json_string); + + RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", json_data); EXPECT_EQ("table_2", table.table_name); EXPECT_TRUE(table.is_single_table); } { - std::string json = R"EOF( + std::string json_string = R"EOF( { "RequestItems": { "table_2": { "test1" : "something" }, @@ -146,13 +126,15 @@ TEST(DynamoRequestParser, parseTableNameBatchOperation) { } } )EOF"; - RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", json); + Json::StringLoader json_data(json_string); + + RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", json_data); EXPECT_EQ("", table.table_name); EXPECT_FALSE(table.is_single_table); } { - std::string json = R"EOF( + std::string json_string = R"EOF( { "RequestItems": { "table_2": { "test1" : "something" }, @@ -160,52 +142,60 @@ TEST(DynamoRequestParser, parseTableNameBatchOperation) { } } )EOF"; - RequestParser::TableDescriptor table = RequestParser::parseTable("BatchWriteItem", json); + Json::StringLoader json_data(json_string); + + RequestParser::TableDescriptor table = RequestParser::parseTable("BatchWriteItem", json_data); EXPECT_EQ("table_2", table.table_name); EXPECT_TRUE(table.is_single_table); } { - RequestParser::TableDescriptor table = RequestParser::parseTable("BatchWriteItem", "{}"); + RequestParser::TableDescriptor table = + RequestParser::parseTable("BatchWriteItem", Json::StringLoader("{}")); EXPECT_EQ("", table.table_name); EXPECT_TRUE(table.is_single_table); } { RequestParser::TableDescriptor table = - RequestParser::parseTable("BatchWriteItem", "{\"RequestItems\":{}}"); + RequestParser::parseTable("BatchWriteItem", Json::StringLoader("{\"RequestItems\":{}}")); EXPECT_EQ("", table.table_name); EXPECT_TRUE(table.is_single_table); } { - RequestParser::TableDescriptor table = RequestParser::parseTable("BatchGetItem", "{}"); + RequestParser::TableDescriptor table = + RequestParser::parseTable("BatchGetItem", Json::StringLoader("{}")); EXPECT_EQ("", table.table_name); EXPECT_TRUE(table.is_single_table); } } TEST(DynamoRequestParser, parseBatchUnProcessedKeys) { - { EXPECT_THROW(RequestParser::parseBatchUnProcessedKeys("{test"), Json::Exception); } + { + EXPECT_THROW(RequestParser::parseBatchUnProcessedKeys(Json::StringLoader("{test")), + Json::Exception); + } { - std::vector unprocessed_tables = RequestParser::parseBatchUnProcessedKeys("{}"); + std::vector unprocessed_tables = + RequestParser::parseBatchUnProcessedKeys(Json::StringLoader("{}")); EXPECT_EQ(0u, unprocessed_tables.size()); } { std::vector unprocessed_tables = - RequestParser::parseBatchUnProcessedKeys("{\"UnprocessedKeys\":{}}"); + RequestParser::parseBatchUnProcessedKeys(Json::StringLoader("{\"UnprocessedKeys\":{}}")); EXPECT_EQ(0u, unprocessed_tables.size()); } { - std::vector unprocessed_tables = - RequestParser::parseBatchUnProcessedKeys("{\"UnprocessedKeys\":{\"table_1\" :{}}}"); + std::vector unprocessed_tables = RequestParser::parseBatchUnProcessedKeys( + Json::StringLoader("{\"UnprocessedKeys\":{\"table_1\" :{}}}")); EXPECT_EQ("table_1", unprocessed_tables[0]); EXPECT_EQ(1u, unprocessed_tables.size()); } { - std::string json = R"EOF( + std::string json_string = R"EOF( { "UnprocessedKeys": { "table_1": { "test1" : "something" }, @@ -213,7 +203,10 @@ TEST(DynamoRequestParser, parseBatchUnProcessedKeys) { } } )EOF"; - std::vector unprocessed_tables = RequestParser::parseBatchUnProcessedKeys(json); + Json::StringLoader json_data(json_string); + + std::vector unprocessed_tables = + RequestParser::parseBatchUnProcessedKeys(json_data); EXPECT_TRUE(find(unprocessed_tables.begin(), unprocessed_tables.end(), "table_1") != unprocessed_tables.end()); EXPECT_TRUE(find(unprocessed_tables.begin(), unprocessed_tables.end(), "table_2") != @@ -225,21 +218,21 @@ TEST(DynamoRequestParser, parseBatchUnProcessedKeys) { TEST(DynamoRequestParser, parsePartitionIds) { { std::vector partitions = - RequestParser::parsePartitions("{}"); + RequestParser::parsePartitions(Json::StringLoader("{}")); EXPECT_EQ(0u, partitions.size()); } { std::vector partitions = - RequestParser::parsePartitions("{\"ConsumedCapacity\":{}}"); + RequestParser::parsePartitions(Json::StringLoader("{\"ConsumedCapacity\":{}}")); EXPECT_EQ(0u, partitions.size()); } { - std::vector partitions = - RequestParser::parsePartitions("{\"ConsumedCapacity\":{ \"Partitions\":{}}}"); + std::vector partitions = RequestParser::parsePartitions( + Json::StringLoader("{\"ConsumedCapacity\":{ \"Partitions\":{}}}")); EXPECT_EQ(0u, partitions.size()); } { - std::string json = R"EOF( + std::string json_string = R"EOF( { "ConsumedCapacity": { "Partitions": { @@ -249,8 +242,10 @@ TEST(DynamoRequestParser, parsePartitionIds) { } } )EOF"; + Json::StringLoader json_data(json_string); + std::vector partitions = - RequestParser::parsePartitions(json); + RequestParser::parsePartitions(json_data); for (const RequestParser::PartitionDescriptor& partition : partitions) { if (partition.partition_id_ == "partition_1") { EXPECT_EQ(1u, partition.capacity_); From a2fd6498b96ee917e441f65aaf8d0286e66a602c Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Mon, 17 Oct 2016 11:38:02 -0700 Subject: [PATCH 09/10] Add spacing --- source/common/dynamo/dynamo_filter.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/dynamo/dynamo_filter.cc b/source/common/dynamo/dynamo_filter.cc index c2dbc16f91b97..5a9ad5f0a79ed 100644 --- a/source/common/dynamo/dynamo_filter.cc +++ b/source/common/dynamo/dynamo_filter.cc @@ -60,6 +60,7 @@ void DynamoFilter::onEncodeComplete(const Buffer::Instance& data) { uint64_t status = Http::Utility::getResponseStatus(*response_headers_); chargeBasicStats(status); + std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data); if (!body.empty()) { try { From 3891f853608ca07ca68e98706e57c61ef5752c8a Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Mon, 17 Oct 2016 12:55:55 -0700 Subject: [PATCH 10/10] Address pr comments --- source/common/dynamo/dynamo_filter.h | 3 ++- source/common/dynamo/dynamo_request_parser.cc | 4 ++++ source/common/dynamo/dynamo_request_parser.h | 3 ++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/source/common/dynamo/dynamo_filter.h b/source/common/dynamo/dynamo_filter.h index 99448b8312929..10ce104dedde5 100644 --- a/source/common/dynamo/dynamo_filter.h +++ b/source/common/dynamo/dynamo_filter.h @@ -2,11 +2,12 @@ #include "dynamo_request_parser.h" -#include "common/json/json_loader.h" #include "envoy/http/filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/stats.h" +#include "common/json/json_loader.h" + namespace Dynamo { /** diff --git a/source/common/dynamo/dynamo_request_parser.cc b/source/common/dynamo/dynamo_request_parser.cc index 0c744fec54f04..5619a35383497 100644 --- a/source/common/dynamo/dynamo_request_parser.cc +++ b/source/common/dynamo/dynamo_request_parser.cc @@ -97,6 +97,10 @@ std::vector RequestParser::parseBatchUnProcessedKeys(const Json::Ob } std::string RequestParser::parseErrorType(const Json::Object& json_data) { std::string error_type = json_data.getString("__type", ""); + if (error_type.empty()) { + return ""; + } + for (const std::string& supported_error_type : SUPPORTED_ERROR_TYPES) { if (StringUtil::endsWith(error_type, supported_error_type)) { return supported_error_type; diff --git a/source/common/dynamo/dynamo_request_parser.h b/source/common/dynamo/dynamo_request_parser.h index 9c78a840d3073..8505f9fcae251 100644 --- a/source/common/dynamo/dynamo_request_parser.h +++ b/source/common/dynamo/dynamo_request_parser.h @@ -1,8 +1,9 @@ #pragma once -#include "common/json/json_loader.h" #include "envoy/http/header_map.h" +#include "common/json/json_loader.h" + namespace Dynamo { /*