DynamoDB: Log stats for partition id per operation by ccaraman · Pull Request #147 · envoyproxy/envoy

ccaraman · 2016-10-13T17:44:13Z

Please note that this is a pre-release Amazon DynamoDB feature that is not yet widely available.

ccaraman · 2016-10-13T17:50:40Z

source/common/dynamo/dynamo_filter.cc

+  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,


lower case 'c' for capacity.

theatrus · 2016-10-13T17:55:47Z

From a stats generation standpoint, this looks fine

RomanDzhabarov · 2016-10-13T17:47:02Z

source/common/dynamo/dynamo_request_parser.h

  };

+  struct PartitionDescriptor {
+    PartitionDescriptor(std::string partition, uint64_t capacity)


pass as const std::string& partition

RomanDzhabarov · 2016-10-13T17:49:45Z

source/common/dynamo/dynamo_filter.cc


    chargeBasicStats(status);

+    chargeTablePartitionIdStats(data);


we'd need to update docs, once review is close to the final state: https://lyft.github.io/envoy/docs/configuration/http_filters/dynamodb_filter.html#config-http-filters-dynamo

nit: remove newline at 60 and 62

RomanDzhabarov · 2016-10-13T17:54:53Z

source/common/dynamo/dynamo_filter.cc

+            stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_);
+        stats_.counter(stats_string).add(partition.capacity_);
+      }
+    } catch (const Json::Exception& jsonEx) {


i'd just remove jsonEx as it's not used.

RomanDzhabarov · 2016-10-13T18:03:05Z

source/common/dynamo/dynamo_filter.h

    encoder_callbacks_ = &callbacks;
  }

+  static std::string buildPartitionStatString(const std::string& stat_prefix,


private to DynamoFilter

I will create a utility class for this method. I want to keep this public to improve testability within dynamo_filter_test.cc

RomanDzhabarov · 2016-10-13T18:05:11Z

source/common/dynamo/dynamo_filter.h

  void chargeUnProcessedKeysStats(const Buffer::Instance& data);
+  void chargeTablePartitionIdStats(const Buffer::Instance& data);
+
+  static const size_t MAX_NAME_SIZE = 127;


risky as this depends on stats const, why not use const from RawStats directly?

Updated to use RawStats var.

RomanDzhabarov · 2016-10-13T18:07:03Z

source/common/dynamo/dynamo_filter.cc

+                                                   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.


is the partition_id uuid4? (wondering what those 7 chars look like)

Yes, the partition_id is a uuid. We are choosing to use the last 7 characters for now.

RomanDzhabarov · 2016-10-13T18:43:47Z

source/common/dynamo/dynamo_filter.cc

+      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;


why do we have here -1, MAX_NAME_SIZE is the actual length we can have in stats name, see RawStatData struct (it allocates MAX_NAME_SIZE+1 for \0).

Our check is size < MAX_NAME_SIZE
https://github.com/lyft/envoy/blob/master/source/common/stats/stats_impl.cc#L19

looks like assert is broken, should be <= MAX_NAME_SIZE.

mattklein123 · 2016-10-13T20:57:43Z

docs/configuration/http_filters/dynamodb_filter.rst


+*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.<stat_prefix>.dynamodb.table.<table_name>.*
+namespace. For Batch operations, Envoy tracks per partition and operation stats in the case only if it is the same


mattklein123 · 2016-10-13T20:58:15Z

docs/configuration/http_filters/dynamodb_filter.rst

+    :header: Name, Type, Description
+    :widths: 1, 1, 2
+
+    capacity.<operation_name>.__partition_id=<last_seven_characters_from_partition_id>, Counter, Total number of


did you preview output? I think this probably has to be on the same line

It does have to be on the same line. Fixed

mattklein123 · 2016-10-13T21:00:23Z

source/common/dynamo/dynamo_filter.cc

+  if (table_descriptor_.table_name.empty() || operation_.empty()) {
+    return;
+  }
+  std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data);


you are potentially going to buildBody() multiple times I think, and now this is always going to happen in general. Let's make sure this only happens once.

buildBody is now being called from onEncodeComplete.

mattklein123 · 2016-10-13T21:00:59Z

source/common/dynamo/dynamo_filter.cc

+            stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_);
+        stats_.counter(stats_string).add(partition.capacity_);
+      }
+    } catch (const Json::Exception& jsonEx) {


delete jsonEx

Deleted for all instances.

mattklein123 · 2016-10-13T21:01:47Z

source/common/dynamo/dynamo_filter.cc

+  std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data);
+  if (!body.empty()) {
+    try {
+      std::vector<Dynamo::RequestParser::PartitionDescriptor> partitions =


Nit, remove Dynamo:: from everywhere in this file. You don't need to specify the namespace prefix within the namespace. Makes code less verbose

Cleaned up all 'Dynamo::' in the code.

mattklein123 · 2016-10-13T21:03:56Z

source/common/json/json_loader.cc

+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_));


mattklein123 · 2016-10-13T21:04:35Z

source/common/json/json_loader.h

+   * @param name supplies the key name.
+   * @param default_value supplies the value to return if name does not exist.
+   * @return double the value.
+   */


just double, const double& not needed.

mattklein123 · 2016-10-13T21:08:12Z

source/common/dynamo/dynamo_request_parser.cc

+  Json::StringLoader json(data);
+  std::vector<RequestParser::PartitionDescriptor> partition_descriptors;
+
+  if (json.hasObject("ConsumedCapacity")) {


You can make this logic less nested by using getObject() with allow_empty=true

mattklein123 · 2016-10-13T21:08:46Z

source/common/dynamo/dynamo_request_parser.cc

+
+      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.


it's not intuitive to me what is going on here. Since this is not yet in the public docs, can you have a slightly larger comment on what the API returns here and how we are processing it

mattklein123 · 2016-10-13T21:10:05Z

source/common/dynamo/dynamo_utility.h

+
+class Utility {
+public:
+  static std::string buildPartitionStatString(const std::string& stat_prefix,


comment (talk about all of the truncations, etc. that are done)

Comments added.

mattklein123 · 2016-10-14T01:31:05Z

source/common/dynamo/dynamo_request_parser.cc

+  Json::StringLoader json(data);
+  std::vector<RequestParser::PartitionDescriptor> partition_descriptors;
+
+  Json::Object consumed_capacity = json.getObject("ConsumedCapacity", true);


local var not needed

mattklein123 · 2016-10-14T01:33:58Z

source/common/dynamo/dynamo_request_parser.cc


+std::vector<RequestParser::PartitionDescriptor>
+RequestParser::parsePartitions(const std::string& data) {
+  Json::StringLoader json(data);


In the common case are we parsing the JSON multiple times?

It is possible. Maybe I should update DynamoFilter::buildBody to return Json and update all of dynamo_request_parser to take in a json object?

mattklein123 · 2016-10-14T01:42:42Z

Also, minor, can you change the PR/commit title to be prefixed with "dynamo: "

RomanDzhabarov · 2016-10-14T17:10:58Z

source/common/dynamo/dynamo_filter.cc

+  if (table_descriptor_.table_name.empty() || operation_.empty()) {
+    return;
+  }
+  if (!body.empty()) {


nit: place body.empty() in the if above, makes code below less nested.

Moved body.empty check to above if.

RomanDzhabarov · 2016-10-14T17:15:53Z

source/common/dynamo/dynamo_request_parser.h

  static bool isBatchOperation(const std::string& operation);

+  /**
+   * Parse the Partition ids and the consumed capacity from the body.


nit: for consistency name method as parsePartitions(const std::string& body) ?

For this file, data is the consistent variable name.

mattklein123 · 2016-10-17T18:47:50Z

source/common/dynamo/dynamo_filter.cc

-    if (RequestParser::isBatchOperation(operation_)) {
-      chargeUnProcessedKeysStats(body);
+  std::string body = buildBody(encoder_callbacks_->encodingBuffer(), data);
+  if (!body.empty()) {


if (body.empty()) {
return;
}

mattklein123 · 2016-10-17T18:50:42Z

source/common/dynamo/dynamo_filter.h


 #include "dynamo_request_parser.h"

+#include "common/json/json_loader.h"


nit: include goes below envoy headers, separated by new line

mattklein123 · 2016-10-17T18:51:54Z

source/common/dynamo/dynamo_request_parser.h

@@ -1,5 +1,6 @@
 #pragma once

+#include "common/json/json_loader.h"


nit: header ordering

mattklein123

lgtm after nits

RomanDzhabarov · 2016-10-17T19:51:09Z

source/common/dynamo/dynamo_request_parser.cc

-        return supported_error_type;
-      }
+std::string RequestParser::parseErrorType(const Json::Object& json_data) {
+  std::string error_type = json_data.getString("__type", "");


if __type is not set on the json, we'll iterate though all supported_error_types.

we could early exit if __type is missing or if error_type after json_data.getString("__type", ""); is empty.

RomanDzhabarov · 2016-10-17T19:52:56Z

lgtm (with small comment), +1

This allows us to start up to 370 VMs. Fixes envoyproxy#136. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

matcher: add PathMatcher and use in routing, jwt and rbac

Signed-off-by: Tony Allen <tony@allen.gg>

This patch adds support for global accepted connection limits. May be configured simultaneously with per-listener connection limits and enforced separately. If the global limit is unconfigured, Envoy will emit a warning during start-up. Global downstream connection count tracking (across all listeners and threads) is performed by the network listener implementation upon acceptance of a socket. The mapping of active socket objects to the actual accepted downstream sockets is assumed to remain bijective. Given that characteristic, the connection counts are tied to the lifetime of the objects. Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Signed-off-by: Tony Allen <tony@allen.gg>

Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Yifan Yang <needyyang@google.com>

Signed-off-by: Tony Allen <tony@allen.gg>

…_reporting_service zh-translation: /intro/arch_overview/upstream/load_reporting_service.rst

Signed-off-by: Mike Schore <mike.schore@gmail.com> Description: Welp. Maybe we need this after all. Risk Level: Low Testing: Saw more ceilf errors and this fixed them again. Signed-off-by: JP Simard <jp@jpsim.com>

Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>

closes #147 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

ccaraman added 3 commits October 13, 2016 10:32

Initial commit of partition id stats

6695db2

Partition Id stats

46732a4

Update comments

3eca229

ccaraman commented Oct 13, 2016

View reviewed changes

RomanDzhabarov reviewed Oct 13, 2016

View reviewed changes

ccaraman added 2 commits October 13, 2016 13:04

Address Roman's pr comments

9ed557d

Add newline

6532da8

mattklein123 reviewed Oct 13, 2016

View reviewed changes

ccaraman added 2 commits October 13, 2016 16:09

Expand comments

1a3197f

fix format

ae1e2e7

mattklein123 reviewed Oct 14, 2016

View reviewed changes

ccaraman changed the title ~~Log stats for partition id per operation~~ DynamoDB: Log stats for partition id per operation Oct 14, 2016

RomanDzhabarov reviewed Oct 14, 2016

View reviewed changes

ccaraman added 2 commits October 17, 2016 11:34

Refactor json parsing.

5887f69

Add spacing

a2fd649

mattklein123 reviewed Oct 17, 2016

View reviewed changes

mattklein123 approved these changes Oct 17, 2016

View reviewed changes

RomanDzhabarov reviewed Oct 17, 2016

View reviewed changes

Address pr comments

3891f85

mattklein123 merged commit 7342353 into master Oct 17, 2016

mattklein123 deleted the shard-id-header branch October 17, 2016 20:42

PiotrSikora added a commit to PiotrSikora/envoy that referenced this pull request Aug 25, 2019

build: increase wee8's VSZ limit to 4TB. (envoyproxy#147)

d20ae4c

This allows us to start up to 370 VMs. Fixes envoyproxy#136. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

JimmyCYJ pushed a commit to JimmyCYJ/envoy that referenced this pull request Feb 14, 2020

Merge pull request envoyproxy#147 from istio/path-matcher

ea73c7f

matcher: add PathMatcher and use in routing, jwt and rbac

mattklein123 pushed a commit that referenced this pull request Jun 30, 2020

overload: Runtime configurable global connection limits (#147)

542f84c

Signed-off-by: Tony Allen <tony@allen.gg>

PiotrSikora pushed a commit that referenced this pull request Jun 30, 2020

overload: Runtime configurable global connection limits (#147)

4597ac8

Signed-off-by: Tony Allen <tony@allen.gg>

aimless404 pushed a commit to aimless404/envoy that referenced this pull request Jun 30, 2020

overload: Runtime configurable global connection limits (envoyproxy#147)

dd56512

Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Yifan Yang <needyyang@google.com>

liuxu623 pushed a commit to liuxu623/envoy that referenced this pull request Jul 14, 2020

overload: Runtime configurable global connection limits (envoyproxy#147)

f616be9

Signed-off-by: Tony Allen <tony@allen.gg>

wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this pull request Jan 23, 2021

Merge pull request envoyproxy#147 from webup/intro/arch/upstream/load…

042045e

…_reporting_service zh-translation: /intro/arch_overview/upstream/load_reporting_service.rst

arminabf pushed a commit to arminabf/envoy that referenced this pull request Jun 5, 2024

chore: updates coraza, fixes tests accordingly (envoyproxy#147)

0d6d8a3

Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>

mathetake added a commit that referenced this pull request Mar 3, 2026

chore: adds PR template (#165)

72f37d5

closes #147 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>


		#include "dynamo_request_parser.h"

		#include "common/json/json_loader.h"

		@@ -1,5 +1,6 @@
		#pragma once

		#include "common/json/json_loader.h"

Conversation

ccaraman commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theatrus commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RomanDzhabarov Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

RomanDzhabarov Oct 13, 2016 •

edited

Loading