Make failure reasons to be flags. #274

RomanDzhabarov · 2016-12-06T04:41:06Z

No description provided.

mattklein123 · 2016-12-06T05:34:41Z

include/envoy/http/access_log.h

   */
-  virtual FailureReason failureReason() const PURE;
+  virtual uint64_t failureReason() const PURE;


This name doesn't make sense anymore. I would do the following:

Rename FailureReason -> ResponseFlag

void setResponseFlag(ResponseFlag flag);

bool getResponseFlag(ResponseFlag flag);

This way it is clear what you are adding flags together and can test if a flag is set. In the future we could add a clearResponseFlag() method if we need it.

i assume that if we change FailureReason -> ResponseFlag we'll also need to change access log formatting to allow specifying %RESPONSE_FLAG% instead of %FAILURE_REASON% which will not be backward compatible. Any concerns around this at this point? (If not then I'll go for ReponseFlag).

Yeah I guess so, which makes it a larger change, but I would probably do the right thing here. I'm open to other name ideas also.

For the access log I would do %RESPONSE_FLAGS% (plural) if that is the name.

Well, that's not too much work anyway, main concern around not to be backward compatible. This is def no problem for us, for others it's a simple config change as well, prob not too much to worry about.

I think it's probably OK right now. I don't think there are too many people using yet and probably even fewer specifying a custom access log config.

RomanDzhabarov · 2016-12-07T07:05:24Z

@mattklein123 addressed comments.

mattklein123 · 2016-12-07T18:22:15Z

include/envoy/http/access_log.h

@@ -43,10 +45,9 @@ class RequestInfo {
  virtual ~RequestInfo() {}

  /**
-   * filter can trigger this callback on failed response to provide more details about
-   * failure.
+   * filter can set response flags.


grammar/better comment

fixed in the next iteration

mattklein123 · 2016-12-07T18:22:22Z

include/envoy/http/access_log.h

@@ -89,9 +90,11 @@ class RequestInfo {
  virtual std::chrono::milliseconds duration() const PURE;

  /**
-   * @return the failure reason for richer log experience.
+   * @return response flags to enrich access log with details around response code. Response flag


grammar/better comment

fixed in the next iteration

mattklein123 · 2016-12-07T18:22:58Z

include/envoy/http/access_log.h

   */
-  virtual FailureReason failureReason() const PURE;
+  virtual bool isSetResponseFlag(ResponseFlag response_flag) const PURE;


getResponseFlag() or isResponseFlagSet()

here i was following convention used in thrift for optional fields, getResponseFlag() should be ok as well.

mattklein123 · 2016-12-07T18:23:30Z

source/common/http/access_log/access_log_formatter.cc

+}
+
+const std::string FilterReasonUtils::toShortString(const RequestInfo& request_info) {
+  std::string result = "";


std::string result;

mattklein123 · 2016-12-07T18:24:10Z

source/common/http/access_log/access_log_formatter.cc

@@ -17,39 +17,73 @@ const std::string FilterReasonUtils::UPSTREAM_CONNECTION_TERMINATION = "UC";
 const std::string FilterReasonUtils::UPSTREAM_OVERFLOW = "UO";
 const std::string FilterReasonUtils::NO_ROUTE_FOUND = "NR";
 const std::string FilterReasonUtils::FAULT_INJECTED = "FI";
+const std::string FilterReasonUtils::DELAY_INJECTED = "DI";


Delete until this is actually used in the code, or use it in the code, and also add to docs.

Can we keep this, because the subsequent PR (from me) is be using it? Or would you like to introduce this as another PR?

We could either have all of the changes in one PR (that's harder to review, too many changes)
or have one change for refactoring (this change, and remove DI) and one change with fully DI support.

I'd like to have two clean changes. Let me know if any concerns.

And then a third one that uses all the two codes. I am fine with that as well.

mattklein123 · 2016-12-07T18:25:17Z

include/envoy/http/access_log.h

  // No failure.
-  None,
+  None = 0x0,


Delete. None is implied by no flags.

this way caller can check if no flags are set otherwise we'd need to have separate method isAnyResponseFlagSet or something along these lines.
Open to remove None if there are better ways of handling situation above.

Where do you actually need to check if None is set. That location is probably now a bug in the face of something like DI.

i'll check this

rshriram · 2016-12-12T19:17:07Z

test/common/http/access_log/access_log_formatter_test.cc

+        .WillByDefault(Return(true));
+    ON_CALL(request_info, isSetResponseFlag(ResponseFlag::FaultInjected))
+        .WillByDefault(Return(true));
+    EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(request_info));


Nit. FI and UT cannot occur together. Whenever a request is aborted (with HTTP codes or connection termination), it does not go through the rest of the processing pipeline.

The only combination of FI and DI that will make sense in a single request is DI,FI (in order). [Delays followed by aborting a request].

this was mostly to test basic functionality for multiple flags, i can place comment that it's not real use case but to test functionality

Conflicts: source/common/router/router.cc test/common/router/router_test.cc

…gs_for_access_log

mattklein123 · 2016-12-14T23:45:17Z

include/envoy/http/access_log.h

@@ -43,10 +41,9 @@ class RequestInfo {
  virtual ~RequestInfo() {}

  /**
-   * filter can trigger this callback on failed response to provide more details about
-   * failure.
+   * each filter can set independent response flag, flags are accumulated.


nit: Start with capital letter

mattklein123 · 2016-12-14T23:47:04Z

source/common/http/conn_manager_utility.cc

  }

  // Compiler enforces switch above to cover all the cases and it's impossible to be here,
  // but compiler complains on missing return statement, this is to make compiler happy.
  NOT_IMPLEMENTED;
 }

+bool ConnectionManagerUtility::isTraceableFailure(


I'm concerned this might get out of date. Should we put in ResponseFlagUtils? I could go either way.

between ConnectionManagerUtility and ResponseFlagUtils i think second one is preferable, will move.

mattklein123 · 2016-12-14T23:49:07Z

test/common/http/access_log/access_log_impl_test.cc

-  Http::AccessLog::FailureReason failureReason() const override { return failure_reason_; }
-  void onFailedResponse(FailureReason failure_reason) override { failure_reason_ = failure_reason; }
+  bool getResponseFlag(ResponseFlag response_flag) const override {
+    return response_flags_ == response_flag;


can do that, just kept simple it here, same with set functionality.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

update opencensus

update minimal go version to 1.21

initial.

4542104

mattklein123 reviewed Dec 6, 2016

View reviewed changes

RomanDzhabarov added 3 commits December 5, 2016 23:05

first part of refactoring.

2c62c6d

Fix.

3bfb522

fix tests.

ba7efd5

Update access_log.rst

947a456

mattklein123 reviewed Dec 7, 2016

View reviewed changes

rshriram reviewed Dec 12, 2016

View reviewed changes

rshriram mentioned this pull request Dec 12, 2016

Use flags for access log when injecting failures #287

Closed

RomanDzhabarov added 9 commits December 14, 2016 12:57

Merge branch 'master' into flags_for_access_log

b6d4fa7

Conflicts: source/common/router/router.cc test/common/router/router_test.cc

getResponseFlag.

65741de

first part.

653aa15

Merge branch 'flags_for_access_log' of github.com:lyft/envoy into fla…

0827bd9

…gs_for_access_log

Refactor.

e71ce43

test.

98833d7

nits.

54800f2

fix.

7d63edf

fix method.

12c6a6c

mattklein123 reviewed Dec 14, 2016

View reviewed changes

moved.

9f5516d

mattklein123 approved these changes Dec 15, 2016

View reviewed changes

RomanDzhabarov merged commit 4332dcf into master Dec 15, 2016

mattklein123 deleted the flags_for_access_log branch December 17, 2016 20:45

lizan pushed a commit to lizan/envoy that referenced this pull request Nov 25, 2019

Fix ABI: all data is pulled by the code in the VM. (envoyproxy#274)

dfb1048

Signed-off-by: John Plevyak <jplevyak@gmail.com>

lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Oct 16, 2020

Merge pull request envoyproxy#274 from bianpengyuan/istio-envoy-oc-1.8

37f7aac

update opencensus

krinkinmu pushed a commit to krinkinmu/envoy that referenced this pull request Nov 1, 2024

Merge pull request envoyproxy#274 from zmiklank/go_version_1.22

f8d02fb

update minimal go version to 1.21

Make failure reasons to be flags. #274

Make failure reasons to be flags. #274

Uh oh!

Conversation

RomanDzhabarov commented Dec 6, 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!

RomanDzhabarov commented Dec 7, 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!

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!

Uh oh!