move LowerCaseString to common for common use by wbpcode · Pull Request #16539 · envoyproxy/envoy

wbpcode · 2021-05-18T05:55:11Z

Signed-off-by: wbpcode comems@msn.com

This PR moved Envoy::Http::LowerCaseString to common so that other modules can implement case insensitive string comparison without deps on Http.

This PR is second sub PR of #16049.

Commit Message:
Additional Description:
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2021-05-18T13:01:49Z

/retest

repokitteh-read-only · 2021-05-18T13:01:53Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16539 (comment) was created by @wbpcode.

see: more, trace.

wrowe · 2021-05-18T22:38:45Z

It seems clang_tidy simply wants a cleaner constructor here;

Writing fixes to clang-tidy-fixes.yaml ...
clang-tidy check failed, potentially fixed by clang-apply-replacements:
Diagnostics:
- BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
  DiagnosticMessage:
    FileOffset: 675
    FilePath: include/envoy/common/case_insensitive.h
    Message: use '= default' to define a trivial copy constructor
    Replacements:
    - {FilePath: include/envoy/common/case_insensitive.h, Length: 21, Offset: 723,
      ReplacementText: ''}
    - {FilePath: include/envoy/common/case_insensitive.h, Length: 2, Offset: 744,
      ReplacementText: = default;}
  DiagnosticName: modernize-use-equals-default
  Level: Warning
- BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
  DiagnosticMessage:
    FileOffset: 767
    FilePath: include/envoy/common/case_insensitive.h
    Message: use '= default' to define a trivial copy-assignment operator
    Replacements:
    - {FilePath: include/envoy/common/case_insensitive.h, Length: 50, Offset: 806,
      ReplacementText: = default;}
  DiagnosticName: modernize-use-equals-default
  Level: Warning

test/extensions/filters/http/fault/fault_filter_integration_test.cc

Signed-off-by: wbpcode <comems@msn.com>

…insensitive

wbpcode · 2021-05-21T02:32:38Z

/retest

repokitteh-read-only · 2021-05-21T02:32:42Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16539 (comment) was created by @wbpcode.

see: more, trace.

…insensitive

wbpcode · 2021-05-24T01:43:45Z

/retest

repokitteh-read-only · 2021-05-24T01:43:50Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16539 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2021-05-26T05:15:36Z

@lizan I think this PR is ready for review. 😃

lizan · 2021-05-27T06:21:13Z

include/envoy/common/case_insensitive.h

+  std::string string_;
+};
+
+template <bool (*V)(absl::string_view)> class ValidatedLowerCaseStr : public LowerCaseStrBase {


I'm not sure why we need this to be a template.

@lizan Because I want to get a protocol-independent lower case string and be able to set a different validator. The template is used so that the validator can be pluggable.

Here are some discussions about this question in original PR #16049.
@jmarantz #16049 (comment)

OK I see, then this should be commented why it is template. and in Http, it should be named like validateHttpLowerCaseString.

TBH my recollection from the.other PR is that there are a few jumbled concepts here and I want to just quickly mention my line of thinking.

IMO if you really need a validator, forget whether there's a compelling reason to prefer

template

virtual method

function-pointer
But if we do use a function-pointer then in IMO it could be a std::function rather than a templatized function pointer.

Also I think once we are plugging in validation, there's no compelling reason to use LowerCaseString just for Tracing's sake, unless there's something about Tracing that wants strings to be lower-cased. IDK whether that's the case.

That said, I have no fundamental objection to moving LowerCaseString to common as long as it doesn't make anything slower for HTTP headers, which are performance critical.

I don't understand why LowerCaseString has to be paired with a validator though. Can't you just leave LowerCaseString exactly like it is, and move it out of http into common? That seems like a legitimate concept on its own. You could make another layer that adds validation if this is helpful in some way.

@lizan I will add some new comments to explain why we need this template. 👌
In Http, I kept the original LowerCaseString name so as not to affect the existing code as much as possible. After all, the name Http::LowerCaseString is already widely used. 🤔

@jmarantz

First, about the performance: This modification will not affect the performance of Http::LowerCaseString since there is no new logic. The introduction of templates may increase the compilation overhead slightly.

Second, about the relationship between validator and LowerCaseString: I provide a class LowerCaseStrBase as a common implementation of LowerCaseString which with no any validation and can be used directly. Then I provide ValidatedLowerCaseStr template just as a helper for who wants to add validation to LowerCaseString, for example, Http.
So that means I've got two layers as you think. One layer is the plain LowerCaseString, and the second layer is the ValidatedLowerCaseStr that can mount a different validator.

@jmarantz At the source code level, it is move. I just kept the previous name LowerCaseString instead of renaming it to ValidatedHttpLowerCaseString or something like that to make PR clean.

using LowerCaseString = Envoy::ValidatedLowerCaseStr<validHeaderString>;

One of the key reasons Trace needs LowerCaseString is performance. But that can be discussed in the new PR.

This PR doesn't really have anything to do with Trace so far. Although the cause is Trace, for now this PR can be seen as a PR that simply strips LowerCaseString out of Http to make it more generic.

OK...sorry, TBH I'm skeptical about the overall strategy and also concerned there may be overhead added to http headers.

But for the moment maybe it'd be better to focus on strategy. Do you want to describe the high level of what you are trying to achieve in a bug or a google doc? It might be easier to iterate there.

Thanks for you comments and patience. @jmarantz

In the original PR #16049, there is the high level design of the general tracing. And I remember I had a more detailed discussion with you in slack earlier.

For now, I think we can try to focus on this PR first. Because this PR don't involve the design of the new general tracing yet, just some pre-work. This is also the reason why I split the original PR into multiple PRs.

I think we can discuss the design of general tracing in detail in the new PR (One or two weeks after the completion of this PR).

For this PR I think the most important points are the following:

whether the PR is clean or not.

whether the PR is able to keep the existing functionality working.

whether the PR can guarantee that the existing performance is not affected.

Whether the PR achieves its purpose: to strip LowerCaseString from Http and get a general LowerCaseString.

I think PR is probably OK but I'll let Lizan review the details. It'd be better not to do it unless it's needed though.

I am not understanding why you need lower-case semantics at all for the new tracers. There may be a good reason but it's not clear. Iterating in a doc may be easier, rather than this PR.

Signed-off-by: wbpcode <comems@msn.com>

…insensitive

wbpcode · 2021-05-28T05:25:19Z

/retest

repokitteh-read-only · 2021-05-28T05:25:22Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16539 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2021-05-28T07:39:03Z

/retest

repokitteh-read-only · 2021-05-28T07:39:07Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16539 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2021-05-28T09:55:00Z

@lizan Some comments are added.

I personally think it might be better to keep the name Http::LowerCaseString in Http as it is already widely used. Keeping the original name by using would make this PR much cleaner. 🤔

jmarantz · 2021-05-28T12:07:45Z

source/extensions/tracers/common/ot/opentracing_driver_impl.cc

  opentracing::expected<void> Set(opentracing::string_view key,
                                  opentracing::string_view value) const override {
-    Http::LowerCaseString lowercase_key{key};
+    Http::LowerCaseString lowercase_key{{key.data(), key.size()}};


should this be moved to a different PR? Doesn't seem like this should be needed in this one.

jmarantz · 2021-05-28T12:41:06Z

include/envoy/common/case_insensitive.h

+  std::string string_;
+};
+
+template <bool (*V)(absl::string_view)> class ValidatedLowerCaseStr : public LowerCaseStrBase {


I think PR is probably OK but I'll let Lizan review the details. It'd be better not to do it unless it's needed though.

I am not understanding why you need lower-case semantics at all for the new tracers. There may be a good reason but it's not clear. Iterating in a doc may be easier, rather than this PR.

wbpcode · 2021-06-01T01:54:35Z

Here is a more detailed doc for general tracing. https://docs.google.com/document/d/1gxk5vfHl-Gt3wiNZ-f46Dwmk2NFiEzFJQugmZmpSXk8/edit?usp=sharing
@jmarantz Thank you very much for your continued patience and help. 🌷

wbpcode · 2021-06-01T01:54:59Z

Personally, I still think this PR is actually not that closely related to general tracing.
Because all people who want to use the lower case semantics, but do not want to rely on Http can benefit from it. It is not specifically designed for general tracing.

jmarantz · 2021-06-07T12:38:33Z

@wbpcode agreed. This PR seems like a good one to apply if we need such a semantic in the future. There's nothing wrong with moving it, and this PR would be a good start to do that. But I'm not sure if there'd be any more users at this point, so maybe we can put this one on hold till then?

And I still think your C-style function-pointer based validation could (a) be a separate PR from the move and (b) could be a lambda for easier use in some cases.

jmarantz · 2021-06-07T12:38:49Z

/wait

github-actions · 2021-07-07T16:03:48Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-07-14T20:01:09Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

wbpcode added 2 commits May 18, 2021 13:32

mv lowercasestring to common for common use

8ff0501

Signed-off-by: wbpcode <comems@msn.com>

fix test

0ecec1b

Signed-off-by: wbpcode <comems@msn.com>

wbpcode requested a review from alyssawilk as a code owner May 18, 2021 12:10

lizan reviewed May 18, 2021

View reviewed changes

test/extensions/filters/http/fault/fault_filter_integration_test.cc Show resolved Hide resolved

fix clang-tidy

82139be

Signed-off-by: wbpcode <comems@msn.com>

alyssawilk assigned lizan May 19, 2021

Merge branch 'main' of https://github.com/envoyproxy/envoy into case-…

468ad2b

…insensitive

Merge branch 'main' of https://github.com/envoyproxy/envoy into case-…

7b04853

…insensitive

wbpcode requested a review from lizan May 26, 2021 05:15

lizan reviewed May 27, 2021

View reviewed changes

wbpcode added 2 commits May 28, 2021 10:39

add comments to validated lowercasestring template

03db6bf

Signed-off-by: wbpcode <comems@msn.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into case-…

cfe8f3a

…insensitive

jmarantz reviewed May 28, 2021

View reviewed changes

wbpcode mentioned this pull request Jun 3, 2021

tracing: general tracing support #16049

Closed

repokitteh-read-only bot added the waiting label Jun 7, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 7, 2021

github-actions bot closed this Jul 14, 2021

Conversation

wbpcode commented May 18, 2021

Uh oh!

wbpcode commented May 18, 2021

Uh oh!

repokitteh-read-only bot commented May 18, 2021

Uh oh!

wrowe commented May 18, 2021

Uh oh!

Uh oh!

wbpcode commented May 21, 2021

Uh oh!

repokitteh-read-only bot commented May 21, 2021

Uh oh!

wbpcode commented May 24, 2021

Uh oh!

repokitteh-read-only bot commented May 24, 2021

Uh oh!

wbpcode commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode May 27, 2021 • 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!

wbpcode May 28, 2021 • 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!

wbpcode May 28, 2021 • 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!

wbpcode May 28, 2021 • 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!

wbpcode commented May 28, 2021

Uh oh!

repokitteh-read-only bot commented May 28, 2021

Uh oh!

wbpcode commented May 28, 2021

Uh oh!

repokitteh-read-only bot commented May 28, 2021

Uh oh!

wbpcode commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jun 1, 2021

Uh oh!

jmarantz commented Jun 7, 2021

Uh oh!

jmarantz commented Jun 7, 2021

Uh oh!

github-actions bot commented Jul 7, 2021

Uh oh!

github-actions bot commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

wbpcode commented May 26, 2021 •

edited

Loading

wbpcode May 27, 2021 •

edited

Loading

wbpcode May 28, 2021 •

edited

Loading

wbpcode May 28, 2021 •

edited

Loading

wbpcode May 28, 2021 •

edited

Loading

wbpcode commented May 28, 2021 •

edited

Loading

wbpcode commented Jun 1, 2021 •

edited

Loading