Automatically delete a branch after PR merge

Is there any reason not to delete it?

👍
However, here's a case where you don't want to delete - e.g. you have a long-running branch the git-flow where you have development, release branches, hotfix branches and then you can create a PR to review what would be merged and merge it, but don't delete the long-running branch.
So I think this could be either be an option in the UI checked by default that you want to merge and delete or an option in the extension for all merges one does.
For the actual button, we have 2 cases where the button would be missing or disabled:

you don't have permission to delete the branch because you are not a collaborator on the repo of the head PR branch
the head branch of the PR is a base branch of another PR - I remember you cannot delete in this case, but not sure what the UI was.

Yeah, it’s auto click the button if it’s found, that’s not an issue.
Hmmm and the long-running branches make this possible feature a bit troublesome. Maybe it can just be part of #865

Closing as duplicate of #1024


Allow enable/disable touch_as_mouse at runtime.

Out with the latest version on crates.io and the main branch. Feel free to open it again if something doesn't feel right. Thanks!


common inline map registry support

Commit Message: common inline map registry support
Additional Description:
To close https://github.com/envoyproxy/envoy/issues/27794.
This PR added a new template class InlineMapRegistry. This template could be used as an alternative of normal hash map to store the key/value pairs. But by this template, you can register some frequently used keys as inline keys and get the
handle for the key. Then you can use the handle to access the key/value pair in the map without even one time hash searching.
This is useful when some keys are frequently used and the keys are known at compile time or bootstrap time. You can get superior performance by using the inline handle. For example, the filter state always uses the filter name as the key and the filter name is known at compile time. By using the inline handle, the filter state could get the key/value pair without any hash.
This template also provides the interface to access the key/value pair by the normal key and the interface has similar searching performance as the normal hash map. But the insertion and removal by the normal key is slower than the normal hash map.
The Scope template parameter is used to distinguish different inline map registry. Different Scope will have different inline key registrations and different scope id.
These is usage example:
  // Define a scope type.
  class ExampleScope {
  public:
    static absl::string_view name() { return "ExampleScope"; }
  };

  // Register the inline key. We should never do this after bootstrapping. Typically, we can
  // do this by define a global variable.
  auto inline_handle = InlineMapRegistry<ExampleScope>::registerInlineKey("inline_key");

  // Create the inline map.
  auto inline_map = InlineMapRegistry<ExampleScope>::InlineMap<std::string>::createInlineMap();

  // Get value by inline handle.
  inline_map->insert(inline_handle, std::make_unique<std::string>("value"));
  EXPECT_EQ(*inline_map->lookup(inline_handle), "value");

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Additional test are necessary.

A quick benchmark of find with 200 short string keys. If normal string key is used, then find of inline map will be a little slower than the normal map. The result is as expected because inline map may do two hash searching (one searching to ensure if it's inline key, one searching for underlay map) for once find or lookup.
If inline handle is used, the inline map is super fast. I am a little doubt this result so I will write some more unit test to ensure everything is right.
Benchmark                  Time             CPU   Iterations
------------------------------------------------------------
InlineMapFind/0/0       4900 ns         4900 ns       146576                    # Normal map.
InlineMapFind/1/0       5369 ns         5369 ns       117522                    # Inline map with normal key.
InlineMapFind/1/1      0.000 ns        0.000 ns<PHONE_NUMBER>                    # Inline map with inline handle.

Can you write more detail in the PR description and class comments when this should be used vs normal maps, including rough performance expectations?
Also, I think this might have been inspired by the HeaderMap implementation; should the HeaderMap implementation be migrated to use this shared infrastructure? Are there pros/cons?

Can you write more detail in the PR description and class comments when this should be used vs normal maps, including rough performance expectations?

I will do it after I complete the unit tests.

Also, I think this might have been inspired by the HeaderMap implementation; should the HeaderMap implementation be migrated to use this shared infrastructure? Are there pros/cons?

The header map is a little complex. It's impossible to replace the current header map impl in short term (I think it's a long term target).
The initial target is using it as underlay container of filter state or per filter config to speed up the searching operations on the key path of request processing.

Should I update the following code to using CONSTRUCT_ON_FIRST_USE?
  static uint64_t registrationMapSize() {
    static uint64_t size = []() {
      finalize();
      return mutableRegistrationMap().size();
    }();

    return size;
  }

I think this object will only be constructed on first call and there should has no static initialization/deinitializatoin order problems for simple uint64_t with copy semantic?

WDYT of, for the moment, putting an application if this new API in Envoys' codebase into the PR, or making a branched PR that adds a usage. It would just be helpful to see the use-case that inspired this as I have opinions about the usage model that would be better informed if I saw what you had in mind.

Thanks for you detailed review. I can make the filter state a example to the usage of the new inline map. I will add some new commit to this PR directly.
And after we complete most reviews and if it's necessary, we can split this PR finally.

What's your thinking about my higher level suggestion to make this class more stl-like and not have any static data at all? Just make it owned by the registry object and leave that in user control.

If it could make this registry easier to user or maintain, I think it's completely OK for me.
But I am not sure I get the stl-like clearly. I think you want the inline map could be used in a way like normal stl containers and has no special thread limitation or requirement?

This is not quite a drop-in replacement for hash_map, but I think you should use hash_map as a starting point. The difference is that you have a registry object that needs to be created. Something like this usage model might make sense:
InlineRegistry ireg;
InlineRegistry::Key key1 = ireg.addKey("key1");
InlineRegistry::Key key2 = ireg.addKey("key2");
InlineRegistry::Map<int> map;
map[key1] = 25;  // inline key
map[key2] = 50;  // inline
map["dynamic key"] = 100;  // dynamic

Then you can do instant lookups using InlineRegistry::Key (which could just be an index), and hash lookups using other strings.
Now if the caller wants to make InlineRegistry or its Keys be singletons or globals or whatever, that's up to them to sort out how to make them thread safe. But I think this basic functionality should be "pure" and not have any hidden state. Let the caller choose the semantics that work for them.

See also https://github.com/envoyproxy/envoy/blob/37f764e056505632ae12a22d215fa46305be4053/source/common/common/utility.h#L764 for some re-usable magic to help factor out management of inline storage.

I think my suggestions are largely in line with you did, but the usage model I suggested requires zero static or global variables, CONSTRUCT_ON_FIRST_USE, or assumptions about main thread architecture.

/wait

but the usage model I suggested requires zero static or global variables, CONSTRUCT_ON_FIRST_USE, or assumptions about main thread architecture.

I am not sure I got it correctly. If I am wrong, feel free to point it out.
You mean we can make the registry a normal factory class. We can create a registry object in any location and add some inline keys to this object in anytime, and then we can also create inline map object by this registry object?
Although it's ok, I think it has some minor shortages (the first one is my concern):

We cannot ensure the validity of inline handle by the type directly. In the current implementation, the registry template will take a scope class as the template parameter. Every registry have different scope class, inline key set and inline handle type. The inline map could only accept the inline handle from same registry. For example, we cannot use B registry's inline handle in A registry's inline map. This point ensure the inline handle always valid and needn't any additional check at runtime.
Additional lifetime management to the registry object is necessary. This registry should have longer lifetime than all inline map objects that created by this registry object.
Still need some helper or CONSTRUCT_ON_FIRST_USE to create some singleton registry objects. Because in some scenarios, we need register inline keys in different modules/filters to same registry.

The implementation is refactored.
The inline map registry now is a normal class. We can create a inline map registry object at any position/thread and use the map registry object to create the inline map objects. Except an atomic integer (used to generate unique registry id for every inline map registry object), no other static variables, global variables or CONSTRUCT_ON_FIRST_USE are introduced.
By this way, we needn't assume the main thread will complete the registration before the inline map registry usage.

Nice! I will look your new refactor later today. I had some thoughts in response to your questions.
First I realize I don't love the word "registry" for the keys. I'm OK with it, but what do you think about the word "descriptor" instead, as a structure that defines the keys. I still like InlineMap or similar for the map.
To your numbered points:

I don't think it's essential to have the compiler guarantee the keys are from the right registry or descriptor. Compilers in C/C++ also do not guarantee vector/array references are in-bounds. However you got me thinking that may be the descriptor can be a variadic template. I haven't sorted out how that would work yet though.
Lifetime management is precisely why I prefer staying away from static constructed objects. It's easy when you are explicitly instantiated to guarantee lifetime and destruction order.
There is no problem with users of this class using whatever memory model they prefer. But I want to remind you of one fact: in Envoy now there is not a single main thread. Envoy Mobile has introduced the concept of multiple independent main threads.

I'm OK with it, but what do you think about the word "descriptor" instead, as a structure that defines the keys. I still like InlineMap or similar for the map.

I have no any preference to the naming. descriptor is good to me. I can update it when I address your following comments.

There is no problem with users of this class using whatever memory model they prefer. But I want to remind you of one fact: in Envoy now there is not a single main thread. Envoy Mobile has introduced the concept of multiple independent main threads.

I am not familiar to the envoy mobile. Then where we will initialize the server? orz

In e-m there are multiple main threads and multiple server objects.
On Tue, Jun 20, 2023, 7:59 AM code @.***> wrote:

I'm OK with it, but what do you think about the word "descriptor" instead,
as a structure that defines the keys. I still like InlineMap or similar for
the map.
I have no any preference to the naming. descriptor is good to me. I can
update it when I address your following comments.
There is no problem with users of this class using whatever memory model
they prefer. But I want to remind you of one fact: in Envoy now there is
not a single main thread. Envoy Mobile has introduced the concept of
multiple independent main threads.
I am not familiar to the envoy mobile. Then where we will initialize the
server? orz
—
Reply to this email directly, view it on GitHub
https://github.com/envoyproxy/envoy/pull/27928#issuecomment-1598639404,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAO2IPP5GQVHGYRJRXJJKOLXMGGCRANCNFSM6AAAAAAZEGNT54
.
You are receiving this because you were mentioned.Message ID:
@.***>

....It's a bit beyond my understanding.
As far as I know, we will lots of important operations at the server initialization, like create runtime singleton, register some inline headers for HTTP header map, create some bootstrap extensions, etc. I would have throught all these operation will done only once and are safe. orz.

The code I gave has a flaw which is why you do need a descriptor: if you use a the Key to write a value that has a preset it will wind up in the map in both the inline vector and the dynamic map. So we need to pre-populate a map. Here's what I was thinking about descriptors, now revived. Note that you don't need any construct_on_first_use to use this. But the caller an elect to make the descriptor using one.
#include <array>
#include <bitset>
#include <functional>

#include "source/common/common/assert.h"

#include "absl/container/flat_hash_map.h"
#include "absl/strings/string_view.h"

template<typename EnumClass, typename Key, typename Value> class InlineMap {
public:
  using KeyArray = std::array<Key, static_cast<size_t>(EnumClass::NumChoices)>;

  class Descriptor {
   public:
    template<typename KeyContainer> Descriptor(const KeyContainer& keys) {
      ASSERT(keys.size() == static_cast<size_t>(EnumClass::NumChoices));
      size_t index = 0;
      for (const Key& key : keys) {
        enum_to_key_[index] = key;
        key_to_enum_[key] = static_cast<EnumClass>(index++);
      }
    }

    KeyArray enum_to_key_;
    absl::flat_hash_map<Key, EnumClass> key_to_enum_;
  };

  InlineMap(const Descriptor& desc) : descriptor_(desc) {}

  Value& operator[](EnumClass key) {
    size_t index = static_cast<size_t>(key);
    inline_is_set_[index] = true;
    return inline_values_[index];
  }
  Value& operator[](Key key) {
    auto iter = descriptor_.key_to_enum_.find(key);
    if (iter != descriptor_.key_to_enum_.end()) {
      return (*this)[iter->second];
    }
    return dynamic_values_[key];
  }
  void foreach(std::function<void(Key, const Value&)> fn) {
    for (size_t i = 0; i < inline_values_.size(); ++i) {
      if (inline_is_set_[i]) {
        fn(descriptor_.enum_to_key_[i], inline_values_[i]);
      }
    }
    for (auto& iter : dynamic_values_) {
      fn(iter.first, iter.second);
    }
  }

private:
  const Descriptor& descriptor_;
  std::bitset<static_cast<size_t>(EnumClass::NumChoices)> inline_is_set_;
  std::array<Value, static_cast<size_t>(EnumClass::NumChoices)> inline_values_;
  absl::flat_hash_map<Key, Value> dynamic_values_;
};

int main() {
  enum class Presets {A, B, C, NumChoices};
  using PresetsMap = InlineMap<Presets, absl::string_view, int>;
  static const PresetsMap::Descriptor desc(PresetsMap::KeyArray{"a", "b", "c"});

  PresetsMap map(desc);
  map[Presets::A] = 5;
  map["d"] = 6;
  map.foreach([](absl::string_view, int) {});}
}

So here's my thinking about a way to tackle this problem. I'm not sure if it meets the needs fully, but has the advantage that no registry or descriptor is required

I think this cannot meets our requirement. This inline keys should could be extended easily in different modules. For example, every HTTP filter could define a inline key for the FilterState inline map.
And we also want to register some additional inline keys according to the bootstrap config when bootstrapping. (Take the per filter config of route as an example. We could register all configured filter names as inline keys, and use inline key as parameter of perFilterConfig() and filterDisabled().)

Also @wbpcode did you detect a problem with the current FilterState implementation e.g. with a flame-graph?

I think that's not really fatal to the idea; but I would like to lean on C++ for type safety instead of asserting numeric ID matching for keys. I think this won't be too hard.

If the inline key could be added at runtime, how can we validate it's validity by type? I didn't figured out this point. 🤔

I was thinking the Key would be a subtype of Descriptor, but it would just
encapsulate an int.
You could assert that the Descriptor doesn't get any new keys added if
there are any live instances of it.
Just want to check, though. Are we doing this because a plain flat_hash_map
is too slow?
On Wed, Jun 21, 2023 at 11:31 AM code @.***> wrote:

I think that's not really fatal to the idea; but I would like to lean on
C++ for type safety instead of asserting numeric ID matching for keys. I
think this won't be too hard.
If the inline key could be added at runtime, how can we validate it's
validity by type? I didn't figured out this point. 🤔
—
Reply to this email directly, view it on GitHub
https://github.com/envoyproxy/envoy/pull/27928#issuecomment-1601062896,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAO2IPPVGWAOXX6SM4OD77LXMMHV5ANCNFSM6AAAAAAZEGNT54
.
You are receiving this because you were mentioned.Message ID:
@.***>

Sure, but flat_hash_map is pretty fast. It'd be good to make sure we are
solving an actual problem with this complex infrastructure.
On Wed, Jun 21, 2023 at 11:47 AM code @.***> wrote:

Also @wbpcode https://github.com/wbpcode did you detect a problem with
the current FilterState implementation e.g. with a flame-graph?
I had made one. But I forgot where it was. I can create new one.
But the most important reason for me to take the filter state as the
example of using inline map is not performance problem. It's because most
keys used by the filter state in the key path of request processing is
pre-defined and it is very cheap and easy to update these keys to inline
keys.
—
Reply to this email directly, view it on GitHub
https://github.com/envoyproxy/envoy/pull/27928#issuecomment-1601088166,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAO2IPJMV7TU3HXJ5C4XCNDXMMJQTANCNFSM6AAAAAAZEGNT54
.
You are receiving this because you were mentioned.Message ID:
@.***>

OK check this model out, which allows dynamic addition of new inline keys prior to any instantiations on a descriptor.
#include <vector>
#include <functional>

#include "source/common/common/assert.h"

#include "absl/container/flat_hash_map.h"
#include "absl/strings/string_view.h"

// Inline descriptor holds information about the inline keys. You need
// to instantiate a descriptor as a static or global, however it is trivially
// constructable; it will not cause static-init fiascos. This is because
// it is a structure that contains only a pointer, iniitalized to null,
// which can be set up by the compiler.
//
// The methods in this structure are intended to be called only by InlineMap.
template<typename Key> struct InlineMapDescriptor {
  struct Impl {
    std::vector<Key> index_to_key_;
    absl::flat_hash_map<Key, uint32_t> key_to_index_;
    std::atomic<uint32_t> ref_count_{0};
  };

  void incRef() {
    if (impl_ == nullptr) {
      impl_ = new Impl;
    }
    ++impl_->ref_count_;
  }

  void decRef() {
    --impl_->ref_count_;
  }

  uint32_t addKey(const Key& key) {
    if (impl_ == nullptr) {
      impl_ = new Impl;
    }

    // We cannot add keys to the descriptor while it has been instantiated.
    ASSERT(impl_->ref_count_ == 0);

    uint32_t index = impl_->index_to_key_.size();
    impl_->index_to_key_[index] = key;
    impl_->key_to_index_[key] = index;
    return index;
  }

  uint32_t size() const { return impl_ ? impl_->index_to_key_.size() : 0; }

  Impl* impl_{nullptr};
};

template<typename Key, typename Value, InlineMapDescriptor<Key>* descriptor> class InlineMap {
public:
  // Encapsulates an integer index to an inlined value, so that it can be
  // referenced safely. These must be created by addKey, declared below, which
  // modifies the descriptor.
  class InlineKey {
   public:
    uint32_t index() const { return index_; }

   private:
    friend InlineMap;
    InlineKey(uint32_t index) : index_(index) {}
    uint32_t index_;
  };

  InlineMap()
      : inline_is_set_(descriptor->size()),
        inline_values_(descriptor->size()) {
    descriptor->incRef();
  }

  ~InlineMap() { descriptor->decRef(); }

  // Adds a new inline key to the descriptor. This cannot be called when there
  // is an instantiated map.
  static InlineKey addKey(const Key& key) {
    return InlineKey(descriptor->addKey(key));
  }

  // Inline key lookup.
  Value& operator[](InlineKey key) {
    inline_is_set_[key.index()] = true;
    return inline_values_[key.index()];
  }

  // Dynamic key lookup.
  Value& operator[](Key key) {
    auto iter = descriptor->impl_->key_to_index_.find(key);
    if (iter != descriptor->impl_->key_to_index_.end()) {
      return (*this)[iter->second];
    }
    return dynamic_values_[key];
  }

  // Call a function with every value.
  void foreach(std::function<void(Key, const Value&)> fn) {
    for (size_t i = 0; i < inline_values_.size(); ++i) {
      if (inline_is_set_[i]) {
        fn(descriptor->impl_->index_to_key_[i], inline_values_[i]);
      }
    }
    for (auto& iter : dynamic_values_) {
      fn(iter.first, iter.second);
    }
  }

private:
  std::vector<bool> inline_is_set_;
  std::vector<Value> inline_values_;
  absl::flat_hash_map<Key, Value> dynamic_values_;
};

InlineMapDescriptor<absl::string_view> presetsDescriptor;

int main() {
  using PresetsMap = InlineMap<absl::string_view, int, &presetsDescriptor>;
  PresetsMap::InlineKey a = PresetsMap::addKey("a");
  PresetsMap::addKey("b");
  PresetsMap::addKey("c");
  PresetsMap map;
  map[a] = 5;
  map["d"] = 6;
  map.foreach([](absl::string_view, int) {});
}

I'm doubling down on wanting to know how if this is solving an actual problem in Envoy. Even though I think the above snippet demos a type-safe inline map.

I have been busy with other things today. I will try to reproduce a flame graph tomorrow.
/wait

/wait

From the simplest benchmark result (HTTP1.1, without any filters except router), the getData consume 0.71% CPU. Not big, but hard to say it's trivial, because this is a function on the key path of request processing.

I was looking into some live traffic and that function does show up though it wasn't that close to the top of the profile I saw. Were you doing some analysis and you found that was a bottleneck for an application? Higher things I see on my profile are  related to the protobuf parsing from xds and the http parser. It obviously depends on the load: codecs will dominate with large requests/responses, and xDS will dominate with frequent control-plane updates. Filter overhead will dominate with lots of tiny requests.
Anyway it does seem worth doing. Have you taken a look at the above comment for an approach where the keys are strongly typed? It does rely on static descriptors but they can be constructed by the compiler and don't require a pre-main() init, and that can be tested. It doesn't require a central registry.

cc @jmarantz
Sorry for all the delayed response. I was trapped by other bussiness these two days and get some free bandwidth untill today. Following are some response to your historical comments.

Have you taken a look at https://github.com/envoyproxy/envoy/pull/27928#issuecomment-1601907059 for an approach where the keys are strongly typed?

Yeah. It's very great. But I didn't get what you mean by the keys are strongly typed.
The comment is almost same with my last commit, except the key type is templated parameter and my key type is only std::string.
In my last commit, I think no central registry or  pre-main() init are necessary for the registry/descriptor.

And after rethinking about naming, I personally think registry may be better than descriptor. First, registry puts more emphasis on the fact that it can be dynamically registered and extended. Second, registry can be aligned with current HTTP header map. WDYT?

Were you doing some analysis and you found that was a bottleneck for an application?

Nope. And I think the flat_hash_map is impossbile to be a bootleneck. 🤣 But I am trying do various optimizations because the #19103. This PR is one of these optimizations. Hope I can improve the performance gradually.

Strongly typed keys as shown in the example in main() in the comment:
  using PresetsMap = InlineMap<absl::string_view, int, &presetsDescriptor>;
  PresetsMap::InlineKey a = PresetsMap::addKey("a");

If you make another inline map with a different descriptor you won't be able to do a lookup with key a since that will be tied to PresetsMap; you'll get a compilation error.
This is a bit less like a registry used elsewhere in Envoy as it's just specific to a type of map, and there's no a central registry of map types.

Got it, seems I misunderstant it at first glance. so a pointer of registry is used as the type argument rather than normal object constructor argument. But this require the registry pointer must be a value known in the compile time.
Why cannot we use a templatized registry directly, just like:
template<class Scope, class Key> class CommonRegistry {
  absl::flat_hash_map>Key, uint64_t>& mutableInlineKeys() {
     CONSTRUCT_ON_FIRST_USE(...);
  }
  class InlineKey {...};

  InineKey addKey(Key key) { ... }
};

template<class Scope, class Key, class Value> class InlineMap {
public:
using SpecificRegistry = CommonRegistry<Scope, Key>;
using InlineKey = SpecificRegistry::InlineKey;

static InlineKey addKey(Key key) {  return SpecificRegistry::addKey(key); }
};

using SpecificInlineMap = InlineMap<std::string /* key type */, std::string /* value type /*, SpecificScope>;

Seems this even better except a CONSTRUCT_ON_FIRST_USE is used?

In addition, I think the way in the above comment still need to resolve some other issues:

The thread safety of static pointer of descriptor/registry. If we add keys in multiple threads, it still dangerous. If we want it's lock-free, we still need ensure the descriptor/registry will not be changed after bootstrapping.
How can we print the debug info (all added inline keys) of the descriptor/registry when bootstrapping envoy?

But this require the registry/descriptor pointer must be a value known in the compile time

Correct -- you need to establish the unique identity of the map at compile-time, but you can add inline keys anytime before the first instance is instantiated. We probably should make that thread-safe if that might happen in multiple threads, since we have to have hold those in the static descriptor, and the descriptor needs to be static as a static pointer is required by the language for using as a template parameter. To do that we can just throw a mutex into the impl.

Why cannot we use a templatized registry class directly, just like:...

We can do a lot of things but one thing I'm never sure of is putting static data declarations in a template class that goes in a header. That seems complicated from a linkage perspective. Why is that better?
And what is the Scope in your snippet?

How can we print the debug info (all added inline keys) of all the descriptors/registrylies when bootstrapping envoy?

It would be easy to add InlineMap<...>::printAllInlineKeys if that's desired. But in my proposal there is no central registry so we can't iterate over all of them. We also can't iterate over all flat_hash_map. I really think of this construct as being an extension of STL and it should behave like that, rather than being tightly integrated into Envoy's semantics.
A layer over the inline map can be added that adds a registry of all map types but I personally haven't seen a compelling need for that. IMO we should first build a pure data structure and then add Envoy-specific semantics in a separate layer.

We can do a lot of things but one thing I'm never sure of is putting static data declarations in a template class that goes in a header. That seems complicated from a linkage perspective. Why is that better?

Because a class template parameter is used rather than static pointer which is used more frequently.

And what is the Scope in your snippet?

An identify class of different registry/class. It's functions is same as the static registry/descriptor pointer which will be used to instantiate a inline map type. But rather than using the static registry/descriptor pointer, I'd like to use a scope class and bind the scope class with a registry/decriptor pointer by a factory function template automatically.

Another point is that I think the type of inline key shouldn't be bound to inline map. Because the inline maps with different value types could use same inline keys.

I agree, using a pointer in a template specification is not common but it is standard C++ (I was reading the C++ doc in templates to discover this), and seems to fit this use-case better (IMO).

An identify class of different registry/class

I am not understanding how this would be used or why that's better, but I'll look at your filter-state usage model a bit more deeply.

Another point is that I think the type of inline key shouldn't be bound to inline map. Because the inline maps with different value types could use same inline keys.

That's an interesting point. I hadn't thought of that. I think we could change this structure easily to have the inline-key type be associated with the descriptor rather than the map.

I am not understanding how this would be used or why that's better, but I'll look at your filter-state usage model a bit more deeply.

The latest impl use didn't use the strong typed inline key, so the scope is not used in that filter-state usage.
Here are some quick pseudo code I just wrote:
// This is almost same with the ConstSingleton, I think is widely used and is safe.
template<class Scope>
Descriptor& getDescriptor() {
    static Descriptor* d = new Descriptor();
    return *d;
}

template<class Scope>
class InlineKey {
public:
  static InlineKey addKey(Key key) { getDescriptor().addKey(key); }

  uint64_t index() const { return entry_index_; }
private:
  uint64_t entry_index_{};
};

template<class Scope, class Value>
class InlineMap {
public:
  using InlineKeyImpl = template InlineKey<Scope>;

  Value* lookup(InlineKeyImpl inline_key) { ... }
};

// Scope class. Just be used as indentify.
class ScopeX {
public:
  static std::string name() { return "scope_x"; }
};
class ScopeY {
public:
  static std::string name() { return "scope_y"; }
};

class FilterStateScope {
public:
  static std::string name() { return "filter_state_scope"; }
};

int main() {
  auto inline_key_1 = InlineKey<ScopeX>::addKey("key_1");
  auto inline_key_2 = InlineKey<ScopeX>::addKey("key_2");
  auto inline_key_3 = InlineKey<ScopeY>::addKey("key_3");

  // map_1 and map_2 could use same inline key.
  InlineMap<ScopeX, std::string> map_1;
  InlineMap<ScopeX, int> map_2;
  InlineMap<ScopeY, std::string> map_3;

  map_1.find(inline_key_1); // ok
  map_2.find(inline_key_1); // ok
  map_3.find(inline_key_1); // compile error.

  InlineMap<FilterStateScope, std::string> filter_state_map_1;
  InlineMap<FilterStateScope, FilterStateObject> filter_state_map_2;

}

Take above pseudo code as example, compare to defining a static descriptor object and using it's pointer in the header file, I think define a scope class is more friendly for the users of the inline map. And they could completely ignore the exist and the inner impl of the descriptor.
For users who want to create a new inline map type, they just need to create a scope class and then use this class as a template argument of InlineMap or InlineKey. Then, everything will work correctly.

I see how your scopes work now. I don't think that's any simpler than just creating a descriptor explicitly; actually it's more lines depending on formatting.
Questions on your requirements:

do you need to retain order in the map? Currently neither your implementation or mine retain order.
do you need to also support insert-by-key? E.g.  HeaderMap allows referencing an O(1) header by string, and in my suggested pattern above supports that, but yours doesn't look like it does. This is important because if not, then the descriptor really just needs to be an index and things get a lot simpler. It doesn't need the map; just a max # of keys.

I don't think that's any simpler than just creating a descriptor explicitly; actually it's more lines depending on formatting.

Haha, seems that there are some different preferences.
I hope it at least is basically ok for you?

I think the fundamental difference is whether there's a single pod static per scope, or whether it's a lazy-initialized static descriptor per scope.
The disadvantage of the lazy-initialized descriptor is that it requires a thread-safe operation each time it's accessed. I think that will be needed in the foreach() implementation where we are using the descriptor to map inline keys to the Key type (e.g. string_view). The reason we are doing this is for performance so it seems sub-optimal to require accessing a static lazy-init object each we need to do any operation which involves the Key.
I'm cheating slightly because I don't anticipate the general need to have thread-safe manipulation of descriptors. If that's necessary, more locks would be needed in the static descriptor approach as well.
I acknowledge that your approach can still be optimally fast during inline-key operations as you should have the integer indexes directly available w/o taking a lock.
The other difference is just a matter of whether you have distinct top-level classes for Key, Scope, Descriptor, and Map. I had suggested just having Descriptor and Map and other needed classes are sub-classes to those. This just keeps the interface tighter and easier to use, and like more STL. This is more aesthetic, and you can get the same performance either way.
I just want to double-check: HeaderMap retains insertion order and uses that for iteration, using a doubly-linked list. That adds some complexity. You do not need that for this class, right? I asked that above and you responded "yeah, because we may get registered inline key by string key." but that appears to be an answer to a different question :)

FWIW here's the version using the descriptor static that allows sharing inline keys between maps with different value-types. Actually this simplifies the implementation and usage model a bit:
#include <vector>
#include <functional>

#include "source/common/common/assert.h"

#include "absl/container/flat_hash_map.h"
#include "absl/strings/string_view.h"

// Inline descriptor holds information about the inline keys. You need
// to instantiate a descriptor as a static or global, however it is trivially
// constructable; it will not cause static-init fiascos. This is because
// it is a structure that contains only a pointer, iniitalized to null,
// which can be set up by the compiler.
//
// The methods in this structure are intended to be called only by InlineMap.
template<typename Key> struct InlineMapDescriptor {
  // Encapsulates an integer index to an inlined value, so that it can be
  // referenced safely. These must be created by addKey, declared below, which
  // modifies the descriptor.
  class InlineKey {
   public:
    InlineKey() : index_(0) {}
    InlineKey(const InlineKey& src) : index_(src.index_) {}
    InlineKey& operator=(const InlineKey& src) {
      if (&src != this) {
        index_ = src.index_;
      }
      return *this;
    }
    uint32_t index() const { return index_; }

   private:
    friend InlineMapDescriptor;
    InlineKey(uint32_t index) : index_(index) {}
    uint32_t index_;
  };

  struct Impl {
    std::vector<Key> index_to_key_;
    absl::flat_hash_map<Key, InlineKey> key_to_inline_key_;
    std::atomic<uint32_t> ref_count_{0};
  };

  void incRef() {
    if (impl_ == nullptr) {
      impl_ = new Impl;
    }
    ++impl_->ref_count_;
  }

  void decRef() {
    --impl_->ref_count_;
  }

  // Adds a new inline key to the descriptor. This cannot be called when there
  // is an instantiated map.
  InlineKey addKey(const Key& key) {
    if (impl_ == nullptr) {
      impl_ = new Impl;
    }

    // We cannot add keys to the descriptor while it instantiated.
    ASSERT(impl_->ref_count_ == 0);

    uint32_t index = impl_->index_to_key_.size();
    impl_->index_to_key_[index] = key;
    InlineKey inline_key(index);
    impl_->key_to_inline_key_[key] = inline_key;
    return inline_key;
  }

  uint32_t size() const { return impl_ ? impl_->index_to_key_.size() : 0; }

  Impl* impl_{nullptr};
};

template<typename Key, typename Value, InlineMapDescriptor<Key>* descriptor> class InlineMap {
public:
  using Descriptor = InlineMapDescriptor<Key>;

  InlineMap()
      : inline_is_set_(descriptor->size()),
        inline_values_(descriptor->size()) {
    descriptor->incRef();
  }

  ~InlineMap() { descriptor->decRef(); }

  // Inline key lookup.
  Value& operator[](typename Descriptor::InlineKey key) {
    inline_is_set_[key.index()] = true;
    return inline_values_[key.index()];
  }

  // Dynamic key lookup.
  Value& operator[](Key key) {
    auto iter = descriptor->impl_->key_to_inline_key_.find(key);
    if (iter != descriptor->impl_->key_to_inline_key_.end()) {
      return (*this)[iter->second];
    }
    return dynamic_values_[key];
  }

  // Call a function with every value.
  void foreach(std::function<void(Key, const Value&)> fn) {
    for (size_t i = 0; i < inline_values_.size(); ++i) {
      if (inline_is_set_[i]) {
        fn(descriptor->impl_->index_to_key_[i], inline_values_[i]);
      }
    }
    for (auto& iter : dynamic_values_) {
      fn(iter.first, iter.second);
    }
  }

private:
  std::vector<bool> inline_is_set_;
  std::vector<Value> inline_values_;
  absl::flat_hash_map<Key, Value> dynamic_values_;
};

using MyDescriptor = InlineMapDescriptor<absl::string_view>;
static MyDescriptor myDescriptor;

int main() {
  MyDescriptor::InlineKey a = myDescriptor.addKey("a");
  myDescriptor.addKey("b");
  myDescriptor.addKey("c");

  using IntMap = InlineMap<absl::string_view, int, &myDescriptor>;
  IntMap imap;
  imap[a] = 5;
  imap["d"] = 6;
  imap.foreach([](absl::string_view, int) {});

  using DMap = InlineMap<absl::string_view, double, &myDescriptor>;
  DMap dmap;
  dmap[a] = 5.5;
  dmap["d"] = 6.8;
  dmap.foreach([](absl::string_view, double) {});

  static constexpr MyDescriptor ensureDescriptorIsConstexprConstructable;
  (void) ensureDescriptorIsConstexprConstructable;
}

Also I think we could drop the static altogether at the cost of an extra 8 bytes in the map, which seems reasonable for your use-case -- not a ton of long-lived maps. In this case the descriptor can be on the stack too: This is really the most elegant approach and is probably what I should've recommended in the first place. No statics, thread safety in control of the caller; just like STL.
#include <vector>
#include <functional>

#include "source/common/common/assert.h"

#include "absl/container/flat_hash_map.h"
#include "absl/strings/string_view.h"

// Inline descriptor holds information about the inline keys. You need
// to instantiate a descriptor as a static or global, however it is trivially
// constructable; it will not cause static-init fiascos. This is because
// it is a structure that contains only a pointer, iniitalized to null,
// which can be set up by the compiler.
//
// The methods in this structure are intended to be called only by InlineMap.
template<typename Key> struct InlineMapDescriptor {
  // Encapsulates an integer index to an inlined value, so that it can be
  // referenced safely. These must be created by addKey, declared below, which
  // modifies the descriptor.
  class InlineKey {
   public:
    InlineKey() : index_(0) {}
    InlineKey(const InlineKey& src) : index_(src.index_) {}
    InlineKey& operator=(const InlineKey& src) {
      if (&src != this) {
        index_ = src.index_;
      }
      return *this;
    }
    uint32_t index() const { return index_; }

   private:
    friend InlineMapDescriptor;
    InlineKey(uint32_t index) : index_(index) {}
    uint32_t index_;
  };

  void incRef() { ++ref_count_; }
  void decRef() { --ref_count_; }

  // Adds a new inline key to the descriptor. This cannot be called when there
  // is an instantiated map.
  InlineKey addKey(const Key& key) {
    // We cannot add keys to the descriptor while it instantiated.
    ASSERT(ref_count_ == 0);

    uint32_t index = index_to_key_.size();
    index_to_key_[index] = key;
    InlineKey inline_key(index);
    key_to_inline_key_[key] = inline_key;
    return inline_key;
  }

  uint32_t size() const { return index_to_key_.size(); }

  std::vector<Key> index_to_key_;
  absl::flat_hash_map<Key, InlineKey> key_to_inline_key_;
  std::atomic<uint32_t> ref_count_{0};
};

template<typename Key, typename Value> class InlineMap {
public:
  using Descriptor = InlineMapDescriptor<Key>;

  explicit InlineMap(InlineMapDescriptor<Key>& descriptor)
      : inline_is_set_(descriptor.size()),
        inline_values_(descriptor.size()),
        descriptor_(descriptor) {
    descriptor_.incRef();
  }

  ~InlineMap() { descriptor_.decRef(); }

  // Inline key lookup.
  Value& operator[](typename Descriptor::InlineKey key) {
    inline_is_set_[key.index()] = true;
    return inline_values_[key.index()];
  }

  // Dynamic key lookup.
  Value& operator[](Key key) {
    auto iter = descriptor_.key_to_inline_key_.find(key);
    if (iter != descriptor_.key_to_inline_key_.end()) {
      return (*this)[iter->second];
    }
    return dynamic_values_[key];
  }

  // Call a function with every value.
  void foreach(std::function<void(Key, const Value&)> fn) {
    for (size_t i = 0; i < inline_values_.size(); ++i) {
      if (inline_is_set_[i]) {
        fn(descriptor_.index_to_key_[i], inline_values_[i]);
      }
    }
    for (auto& iter : dynamic_values_) {
      fn(iter.first, iter.second);
    }
  }

private:
  std::vector<bool> inline_is_set_;
  std::vector<Value> inline_values_;
  absl::flat_hash_map<Key, Value> dynamic_values_;
  Descriptor& descriptor_;
};

int main() {
  using MyDescriptor = InlineMapDescriptor<absl::string_view>;
  MyDescriptor my_descriptor;
  MyDescriptor::InlineKey a = my_descriptor.addKey("a");
  my_descriptor.addKey("b");
  my_descriptor.addKey("c");

  using IntMap = InlineMap<absl::string_view, int>;
  IntMap imap(my_descriptor);
  imap[a] = 5;
  imap["d"] = 6;
  imap.foreach([](absl::string_view, int) {});

  using DMap = InlineMap<absl::string_view, double>;
  DMap dmap(my_descriptor);
  dmap[a] = 5.5;
  dmap["d"] = 6.8;
  dmap.foreach([](absl::string_view, double) {});
}

also I didn't put this in the example but I think we should inline the array for inline keys into the map using InlineStorage. It means you have to construct the Map with a factory method but you then save 16 bytes (I think) compared to using a std::vector, and a level of indirection when referencing an inline item.

cc @jmarantz
First of all, I wanna to express my appreciate to your all comments and suggestions. All of them are valuable. And yesterday was weekend, you sill paid your time to this PR. Thanks again. 🌹

There are lots of comments and have two new example pseudo code snippets. So I will repond one by one.

I think the fundamental difference is whether there's a single pod static per scope, or whether it's a lazy-initialized static descriptor per scope.

It's a lazy-initialized static discriptor per scope. Because we need consider the case where users created static inline keys.  lazy-initialized static descriptor per scope could avoids static initialization order problem.

The disadvantage of the lazy-initialized descriptor is that it requires a thread-safe operation each time it's accessed.

I know it's not free. Every access need additional check whether the static pointer is initialized or not. But from the benchmark, it's almost as fast as normal reference. And the widely used ConstSingleton have proved the overhead is very trivial.

I just want to double-check: HeaderMap retains insertion order and uses that for iteration, using a doubly-linked list. That adds some complexity. You do not need that for this class, right? I asked that above and you responded "yeah, because we may get registered inline key by string key." but that appears to be an answer to a different question :)

Sorry, I mis-understood you quesion. We needn't keep the order for now. It's a map.

Response to your first new added pseudo code https://github.com/envoyproxy/envoy/pull/27928#issuecomment-1605438155
What will happed If users want to defined a static inline key? I think there may be static initialization problem?
And I think this implementation lost the strong typed inline keys? Now, the inline keys from different descriptors may be mis-used?

Response to your second new added pseudo code https://github.com/envoyproxy/envoy/pull/27928#issuecomment-1605457729
This is best one. And I knew you had recommended it. And in fact, I had completed it and commited it based you previous suggessions. Check the code https://github.com/envoyproxy/envoy/pull/27928/files#diff-6067665aa8c19d477d205896eb26daa7ae43fcfa7146ff40e7874659bde54083R51-R323
The problem of this implementation is also has no strong typed inline keys, so we cannot throw a error at compile time if the inline key is mis-used. But a registry/descriptor id is used to make sure at least mis-used inline key will not result in runtime error.
In this implementation, we have a purely and clean code base. No statics (except the descriptor id generator), thread safety in control of the caller, no much envoy context dependency. And it's easy to test because no much static declarations are necessary.

In conclusion, we have two directions. One has strong typed inline keys but statics are unavoidable. Another one has no strong typed inline keys, but additional descriptor/registry id checking is necessary.
(The latest commit of this PR has implemented the second one.)

I'll take a look tomorrow, but my last comment has strongly typed inlined keys and no statics.
It isn't the lazy-init that's the problem it's the lock that it may require. That might not come out in your benchmark which likely only has one thread, but with a high core count it might matter.. But really I just want to have a clean map implementation. You can always add lazy-init on top of it to manage keys or whatever; that's up to the application. The library shouldn't need that.

I think both two new add pseudo code snippets have no strong typed inline keys? For all descriptors (assume the Key type is string), they have same inline key type: InlineMapDescriptor<std::string>::InlineKey?
For example:
InlineMapDescriptor<std::string> a;
InlineMapDescriptor<std::string> b;

// typedid(inline_key_a) == typedid(inline_key_b);
auto inline_key_a = a.addKey("key_a");
auto inline_key_b = b.addKey("key_b");

Oh good catch! I missed that I had dropped the type-safety when only templatizing on the key type. Easily fixed though by incorporating your scope idea, and templatizing the descriptor on that also.
#include <vector>
#include <functional>

#include "source/common/common/assert.h"

#include "absl/container/flat_hash_map.h"
#include "absl/strings/string_view.h"

// The Descriptor holds information about the inline keys.
//
// The methods in this structure are intended to be called only by InlineMap.
template<typename KeyType, typename Scope = void> struct InlineMapDescriptor {
  using Key = KeyType;

  // Encapsulates an integer index to an inlined value, so that it can be
  // referenced safely. These must be created by addKey, declared below, which
  // modifies the descriptor.
  class InlineKey {
   public:
    InlineKey() : index_(0) {}
    InlineKey(const InlineKey& src) : index_(src.index_) {}
    InlineKey& operator=(const InlineKey& src) {
      if (&src != this) {
        index_ = src.index_;
      }
      return *this;
    }
    uint32_t index() const { return index_; }

   private:
    friend InlineMapDescriptor;
    InlineKey(uint32_t index) : index_(index) {}
    uint32_t index_;
  };

  void incRef() { ++ref_count_; }
  void decRef() { --ref_count_; }

  // Adds a new inline key to the descriptor. This cannot be called when there
  // is an instantiated map.
  InlineKey addKey(const Key& key) {
    // We cannot add keys to the descriptor while it instantiated.
    ASSERT(ref_count_ == 0);

    uint32_t index = index_to_key_.size();
    index_to_key_[index] = key;
    InlineKey inline_key(index);
    key_to_inline_key_[key] = inline_key;
    return inline_key;
  }

  uint32_t size() const { return index_to_key_.size(); }

  std::vector<Key> index_to_key_;
  absl::flat_hash_map<Key, InlineKey> key_to_inline_key_;
  std::atomic<uint32_t> ref_count_{0};
};

template<class Descriptor, typename Value> class InlineMap {
public:
  using Key = typename Descriptor::Key;

  explicit InlineMap(Descriptor& descriptor)
      : inline_is_set_(descriptor.size()),
        inline_values_(descriptor.size()),
        descriptor_(descriptor) {
    descriptor_.incRef();
  }

  ~InlineMap() { descriptor_.decRef(); }

  // Inline key lookup.
  Value& operator[](typename Descriptor::InlineKey key) {
    inline_is_set_[key.index()] = true;
    return inline_values_[key.index()];
  }

  // Dynamic key lookup.
  Value& operator[](Key key) {
    auto iter = descriptor_.key_to_inline_key_.find(key);
    if (iter != descriptor_.key_to_inline_key_.end()) {
      return (*this)[iter->second];
    }
    return dynamic_values_[key];
  }

  // Call a function with every value.
  void foreach(std::function<void(Key, const Value&)> fn) {
    for (size_t i = 0; i < inline_values_.size(); ++i) {
      if (inline_is_set_[i]) {
        fn(descriptor_.index_to_key_[i], inline_values_[i]);
      }
    }
    for (auto& iter : dynamic_values_) {
      fn(iter.first, iter.second);
    }
  }

private:
  std::vector<bool> inline_is_set_;
  std::vector<Value> inline_values_;
  absl::flat_hash_map<Key, Value> dynamic_values_;
  Descriptor& descriptor_;
};

int main() {
  class MyScope {};
  using MyDescriptor = InlineMapDescriptor<absl::string_view, MyScope>;
  MyDescriptor my_descriptor;
  MyDescriptor::InlineKey a = my_descriptor.addKey("a");
  my_descriptor.addKey("b");
  my_descriptor.addKey("c");

  using IntMap = InlineMap<MyDescriptor, int>;
  IntMap imap(my_descriptor);
  imap[a] = 5;
  imap["d"] = 6;
  imap.foreach([](absl::string_view, int) {});

  using DMap = InlineMap<MyDescriptor, double>;
  DMap dmap(my_descriptor);
  dmap[a] = 5.5;
  dmap["d"] = 6.8;
  dmap.foreach([](absl::string_view, double) {});

  using MyDescriptor2 = InlineMapDescriptor<absl::string_view>;
  MyDescriptor2 my_descriptor2;
  using IntMap2 = InlineMap<MyDescriptor2, int>;
  IntMap2 imap2(my_descriptor2);
  //imap2[a] = 5;  TYPE ERROR
  imap.foreach([](absl::string_view, int) {});
}

Oh good catch! I missed that I had dropped the type-safety when only templatizing on the key type. Easily fixed though by incorporating your scope idea, and templatizing the descriptor on that also.

I think this cannot help? Because by this way, we cannot ensure the one scope is strickly related to only one descriptor?
class MyScope {};
InlineMapDescriptor<std::string, MyScope> a;
InlineMapDescriptor<std::string, MyScope> b;

// typedid(inline_key_a) == typedid(inline_key_b);
auto inline_key_a = a.addKey("key_a");
auto inline_key_a_2 = a.addKey("key_a_2");
auto inline_key_b = b.addKey("key_b");

renaming complete. and the underlay storage is changed to inline uint8_t[]. (Value[] is not allowed by the compiler).
Finally, I think the only postion that we cannot get an concensus is the static init. But I think we can land the core library part first.

/wait (for #28168 to merge)

/retest

This is a  old PR to show how the inline map lib works, but I think it's too old and the CI cannot pass.

I want to be more explicit with a recipe for how I would do this. You made the claim that doing this properly (w/o building complex structures before main()) would touch too many files and I think you have the right to ask me to explain how I think it should be done. So I did a little digging.
I think you need access to the inline keys in FilterStateImpl, but it's got no context. I think that's resolveable in these steps:

Create FilterState::Context object, in which we construct a map descriptor. I think this object can be owned by Server::Instance, similar to Http::Context and Router::Context. See source/server/server.h:392. I think we will know all of the linked-in extensions when Server::InstanceImpl ctor runs.
We will add a reference to FiltertStateContext into Http::Context, and add interfaces to that context to get the FilterState::Context&.
FilterStateImpl is created in three places: ./common/stream_info/filter_state_impl.cc (in a few places) where we create filter states from other filter states, ./common/http/conn_manager_impl.cc, and ./common/router/upstream_request.cc. The HTTP one is easy because Http::Context is already passed to the ConnectionManagerImpl ctor, so you can now get the FilterState::Context from the Http::Context at the call-site. You will need to add an http_context_ member var to that class; the context is already passed to its constructor but it's used to construct the user_agent_ and is not currently saved.
UpstreamRequest has an extra step. However I think it's not too bad. The Http::Context is already passed to the constuctor for Router::Filter and Router::FilterConfig, and is available as interfactes on those objects. I think it won't be too hard to pass it it to the place where the FilterState is constructed.

So there are a few changes needed to plumb this through but I think they are all very easy and non-controversial, following the established pattern of creating the Http::Context. The hardest part (to me, since I don't know how to do it) is knowing exactly which inline keys to declare in the Server.

/wait

As you know I don't love this singleton pattern, and the way it's expressed here seems more fine-grained than necessary.
I'm not completely convinced you need to do it this way. Can we look for a way to use existing Factory objects to store the inline keys?

Yeah, I know you prefer the context pattern. I have thought it in deep. But there are two reasons prevent me to use it:

Huge refactor: as you know, filter state and stream info is used very very widely. No matter a new context or exist context is used to store the inline keys, we need propagate the context to almost everywhere. This will bring super lots of code modifications.
Discrete inline key registration/declaration: I hope third party extensions could register or declare new inline keys in their code and provide a easy way to refer or use the inline handle. If context pattern is used, this target would be hard to achieve.

Considering above two problems, I think RIO of context parttern is relatively low, singleton parttern is cheap and simple and has proved could work well (proved by HTTP inline headers).

I want to be more explicit with a recipe for how I would do this. You made the claim that doing this properly (w/o building complex structures before main()) would touch too many files and I think you have the right to ask me to explain how I think it should be done. So I did a little digging.
I think you need access to the inline keys in FilterStateImpl, but it's got no context. I think that's resolveable in these steps:

Create FilterState::Context object, in which we construct a map descriptor. I think this object can be owned by Server::Instance, similar to Http::Context and Router::Context. See source/server/server.h:392. I think we will know all of the linked-in extensions when Server::InstanceImpl ctor runs.
We will add a reference to FiltertStateContext into Http::Context, and add interfaces to that context to get the FilterState::Context&.
FilterStateImpl is created in three places: ./common/stream_info/filter_state_impl.cc (in a few places) where we create filter states from other filter states, ./common/http/conn_manager_impl.cc, and ./common/router/upstream_request.cc. The HTTP one is easy because Http::Context is already passed to the ConnectionManagerImpl ctor, so you can now get the FilterState::Context from the Http::Context at the call-site. You will need to add an http_context_ member var to that class; the context is already passed to its constructor but it's used to construct the user_agent_ and is not currently saved.
UpstreamRequest has an extra step. However I think it's not too bad. The Http::Context is already passed to the constuctor for Router::Filter and Router::FilterConfig, and is available as interfactes on those objects. I think it won't be too hard to pass it it to the place where the FilterState is constructed.

So there are a few changes needed to plumb this through but I think they are all very easy and non-controversial, following the established pattern of creating the Http::Context. The hardest part (to me, since I don't know how to do it) is knowing exactly which inline keys to declare in the Server.

I had figured out a similar path. Except the step 3 is different. There are not only three places to update. Note, the FilterStateImpl is used in the StreamInfoImpl. And all places that used StreamInfoImpl need to be updated. The health checkers, the connection creations, the dubbo/thrift/generic proxy, the async http/grpc client, etc.

OK I'll look at the singletons later to try to make sure they are as clean as possible.
In the meantime, did you wind up with a perf test for the impact of using inline maps here? It's a ton of complexity being added; want to make sure it's perf benefit is worth it.

In the meantime, did you wind up with a perf test for the impact of using inline maps here? It's a ton of complexity being added; want to make sure the perf benefit is worth it.

I think we have made benchmark tests for the inline map directly. So, new benchmark tests for filter state should make no big difference.
I will run the envoy binary directly to see the cpu overhead of new filter state.

I don't think the inline map benchmarks tell us much detail about the impact on system performacne. If after all this we get <1% I have to ask if the complexity is worth it.

I don't think the inline map benchmarks tell us much detail about the impact on system performacne. If after all this we get <1% I have to ask if the complexity is worth it.

I mean run the benchmark test for the single filter state class make no big sense because we have did the benchmark test for the inline map. So I will run the binary to compare the actual cpu overhead of filter state.

if the complexity is worth it

Back to the complexity, if sington pattern is used, I think the inline map should be pretty simple to use.

/wait

RE "Back to the complexity, if sington pattern is used, I think the inline map should be pretty simple to use." -- I disagree. The code changes are bounded in scope but the complexity of maintaining a codebase with a ton of complex things happening before main() is a high cost. I want to make sure it works it.


Store inputs on analyses objects

To aid in searching analyses objects, we will store the job inputs as file objects on the analyses object.

I'm not thrilled by duplication, but Megan's convinced me for now. Diff tentatively looks good but I defer to @rentzso for review.
If we merge this, I would like to see a ticket filed that tracks either the removal of this duplication or the conclusion that we'll keep duplicating in this & other cases. My understanding is that empowering the mongo connector to use the core (import via requirements.txt? merge repos?) could avoid this in the long run.

@kofalt another option you and I discussed was the possibility of the mongo-es replicator calling the API to get the correct format it needs.

To clarify, one of the reasons for this change is that it gives a more consistent and self-contained format to each analysis in the database.
This enable tools like mongo connectors to read directly the mongo oplog, and to replicate asynchronously but almost instantly the state of the database without any additional load on the API (@megan calling the API to replicate all the records could be ok, but I think it would be better to avoid any additional overload, and also someone will have to write all the code, almost from scratch, to export data from mongo to elasticsearch).
I am not sure that the merge solution will be very practical either: let's say that we need another tool that reads from the oplog. This tool code will also need to be merged to core, and so on...
Data duplication seems the lesser evil, but @kofalt feel free to open a ticket to investigate on this and on the possible implications of data duplication.

LGTM!

Retested the db update script, everything looks good. Merging now.


[Service Bus] Attempt to settle msg after session lock expiry yields unhelpful error messages

In a session enabled queue, completing message right after session lock expiry results in the error This message has been already settled. from https://github.com/Azure/azure-sdk-for-js/blob/0f05661cced8b0ed9cd036b7a13030499d185aa1/packages/%40azure/servicebus/data-plane/lib/serviceBusMessage.ts#L891
Where as completing the message after a little while of the session lock expiry results in the error ``Cannot find the receiver with name ...` from https://github.com/Azure/azure-sdk-for-js/blob/0f05661cced8b0ed9cd036b7a13030499d185aa1/packages/%40azure/servicebus/data-plane/lib/serviceBusMessage.ts#L895
Neither is very helpful to the user.

We need to understand 2 things here
- Understand how the `remote_settled` property got set to `true`
- Understand how the there is no more receiver

We were not providing enough time in the delay here https://github.com/Azure/azure-sdk-for-js/blob/master/packages/%40azure/servicebus/data-plane/test/renewLockSessions.spec.ts#L614
to receive the message. Hence It was running https://github.com/Azure/azure-sdk-for-js/blob/master/packages/%40azure/servicebus/data-plane/test/renewLockSessions.spec.ts#L615 before completing the message.
Current observation:
If we provide enough time inside delay It should give an error but Instead of giving an error it is completing the message and removing it from the queue.
Sample test code
async function testAutoLockRenewalConfigBehavior(
  senderClient: QueueClient | TopicClient,
  receiverClient: QueueClient | SubscriptionClient,
  options: AutoLockRenewalTestOptions
): Promise<void> {
  let numOfMessagesReceived = 0;
  console.log("Test Started.............");

  await senderClient.getSender().send(testMessage);

  const sessionClient = await receiverClient.getSessionReceiver({
    sessionId: testSessionId,
    maxSessionAutoRenewLockDurationInSeconds: options.maxSessionAutoRenewLockDurationInSeconds
  });
  await sessionClient.receive(
    async (brokeredMessage: ServiceBusMessage) => {
      if (numOfMessagesReceived < 1) {
        numOfMessagesReceived++;

        should.equal(brokeredMessage.body, testMessage.body);
        should.equal(brokeredMessage.messageId, testMessage.messageId);

        // Sleeping...
        await delay(
          options.delayBeforeAttemptingToCompleteMessageInSeconds * 1000 +
            lockDurationInMilliseconds +
            1000
        );
        console.log("calling complete.............");
        await brokeredMessage.complete();
      }
    },
    onError,
    {
      autoComplete: false
    }
  );
  await delay(
    options.delayBeforeAttemptingToCompleteMessageInSeconds * 1000 +
      lockDurationInMilliseconds +
      1000 +
      4000
  );

  console.log("calling close............................");
  await sessionClient.close();

  if (uncaughtErrorFromHandlers) {
    chai.assert.fail(uncaughtErrorFromHandlers.message);
  }

  should.equal(numOfMessagesReceived, 1, "Mismatch in number of messages received");
}

So, looks like Service Bus is allowing us to complete a message even after the session has expired.
But, we dont see that happening in testBatchReceiverManualLockRenewalErrorOnLockExpiry which gives the Cannot find the receiver with name error.
When debugging testBatchReceiverManualLockRenewalErrorOnLockExpiry, I found that when the session expired, there was a receiver link close event fired and _onAmqpClose gets called which then closes the receiver object in the SDK resulting in the Cannot find the receiver with name
In conclusion, on session lock expiry

in receiveBatch(), the underlying receiver link is getting closed, thus stopping us from attempting to complete the message
in receive(), Service Bus is able to complete the message just fine

This needs more digging from our side to see what is the difference between the 2 cases

On enabling debug logs, I see the following error appear in logs that seems to be returned to us by service bus
rhea:frames [connection-2]:3 <- detach#16 {"closed":true,"error":{"condition":"com.microsoft:session-lock-lost","description":"The session lock has expired on the MessageSession. Accept a new MessageSession. TrackingId:e5f257dd0000d66e000377895c54f867_G22_B40, SystemTracker:standardfruitsservivebus:Queue:unpartitioned-sessions-queue, Timestamp:2019-02-02T01:55:17"}}

azure:service-bus:error [connection-2] An error occurred for Receiver 'unpartitioned-sessions-queue-b9e14f9a-86da-b641-941f-a982c2f6e770': { SessionLockLostError: The session lock has expired on the MessageSession. Accept a new MessageSession. TrackingId:e5f257dd0000d66e00037ad45c54fc29_G38_B40, SystemTracker:standardfruitsservivebus:Queue:unpartitioned-sessions-queue, Timestamp:2019-02-02T02:11:19
    at Object.translate (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\@azure\amqp-common\dist\lib\errors.js:527:17)
    at Receiver.MessageSession._onAmqpError (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\test-dist\index.js:13415:44)
    at Receiver.emit (events.js:182:13)
    at emit (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea-promise\dist\lib\util\utils.js:129:24)
    at Object.emitEvent (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea-promise\dist\lib\util\utils.js:140:9)
    at Receiver._link.on (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea-promise\dist\lib\link.js:249:25)
    at Receiver.emit (events.js:182:13)
    at Receiver.link.dispatch (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\link.js:59:37)
    at Receiver.link.on_detach (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\link.js:159:32)
    at Session.on_detach (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\session.js:718:27)
    at Connection.(anonymous function) [as on_detach] (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\connection.js:737:30)
    at c.dispatch (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\types.js:909:33)
    at Transport.read (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\transport.js:108:36)
    at SaslClient.read (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\sasl.js:299:26)
    at Connection.input (c:\workspace\newrepo2\azure-sdk-for-js\packages\@azure\servicebus\data-plane\node_modules\rhea\lib\connection.js:491:35)
    at TLSSocket.emit (events.js:182:13)
    at addChunk (_stream_readable.js:283:12)
    at readableAddChunk (_stream_readable.js:264:11)
    at TLSSocket.Readable.push (_stream_readable.js:219:10)
    at TLSWrap.onStreamRead [as onread] (internal/stream_base_commons.js:94:17)
  name: 'SessionLockLostError',
  translated: true,
  retryable: false,
  info: null,
  condition: 'com.microsoft:session-lock-lost' }.

However this appears only when running the test
it.only(` (First) Receive a msg using Batch Receiver, wait until its lock expires, completing it now results in error`, async function(): Promise<
        void
      > {
        await testBatchReceiverManualLockRenewalErrorOnLockExpiry(senderClient, receiverClient);
      });

first, followed by
it.only(" (Second) Receives a message using Streaming Receiver renewLock() resets lock duration each time.", async function(): Promise<
        void
      > {
        await testAutoLockRenewalConfigBehavior(senderClient, receiverClient, {
          maxSessionAutoRenewLockDurationInSeconds: 0,
          delayBeforeAttemptingToCompleteMessageInSeconds: 31
        });
        // Service bus completes the message even when the session lock expires.
      });

The tests would pass, however the logs reveal the error, but only in case of streaming receiver test.
Running the streaming receiver test alone did not surface the error.
Overall, need to take a more closer look at what caused service bus to return the SessionsLockLostError and when.
cc @HarshaNalluru, @ramya0820, @ShivangiReja

@ramya0820 If I understood this correctly, you are saying that the SessionLockLostError error is seen only when the test that attempts to complete the message after waiting lock to expire when running the batching receiver is followed by the test that does the same with streaming receiver.
From what I have seen, the same error occurs when running the test for batching receiver independently as well.
The order of the tests being run here isnt the issue. The issue is that on lock expiry, the underlying session receiver gets closed after encountering the SessionLockLostError error. But this only happens when using the batching receiver and not the streaming receiver.

Progress since last update:
Got streaming receiver to trigger the SessionLockLostError event by trialing changes that generally mimic batching receiver's flow.
With https://github.com/Azure/azure-sdk-for-js/pull/1128 ,it currently surfaces the SessionLockLostError upto the user's custom onError handler and is triggered asynchronously, not when invoking the complete() on message.
Program exits and message handler gets executed after that, eventually erroring out with a receiver not found in background.
Overall, following are list of issues/concerns to be addressed (looking at them in given order):

Batching Receiver's onError handler is invoked, but that may need similar re-visit as well as it's current error handling mechanism does not surface the appropriate error.
Session Lock renewal tests will need to be updated accordingly.
Re-visit error handling in streaming receiver with sessions, as try catch block around onMessage are no-op/not useful during asynchronous onError event triggers.
May require further deep-dive into how credits and credit window work.

Batching Receiver's onError handler is invoked, but that may need a re-visit as it's current error handling mechanism does not surface the appropriate error.

By the time the error occurs, the promise returned by receiveBatch is resolved. Therefore, there is literally no way to notifiy the user as soon as the session lock lost error occurs. Which is fine. As long as any further user engagement with the session receiver returns this error, we should be good.
We can probably do this by keeping track of all session ids for which session lock lost error is seen. And then the SDK throwing the SessionLockLost error when user tries to settle a message

Re-visit error handling in streaming receiver with sessions, as try catch block around onMessage are no-op/not useful during asynchronous onError event triggers

The try/catch around onMessage is only useful when there is an incoming message. The session lock lost error cannot surface here. But, it should surface from the _onAmqpError which should call notifyError which should call the user's error handler

@ramya-rao-a
receiveBatch() default timeout for first message is currently 60 seconds, so it seems reasonable to expect promise get rejected at 30 second mark or so if auto lock renewal is disabled?
About throwing the error on message settling, agree that seems reasonable.. or maybe we could make use of the sessionLockedUntilUtc property.

The try/catch around onMessage is only useful when there is an incoming message. The session lock lost error cannot surface here. But, it should surface from the _onAmqpError which should call notifyError which should call the user's error handler

Yeah, I think the streaming receiver implementation is good for most part actually.

receiveBatch() default timeout for first message is currently 60 seconds, so it seems reasonable to expect promise get rejected at 30 second mark or so if auto lock renewal is disabled?

Yes, but that will only happen if the expiry happens in those 60 seconds. I believe our test cases run the receiveBatch() call before the expiry and then call complete() on the message after expiry.

or maybe we could make use of the sessionLockedUntilUtc property.

This has 2 issues

To use the sessionLockedUntilUtc property, you need to have access to the receiver. But our problem is that the reciever is already closed and nullified by the time complete() is called after session expiry
Even if we manage to cache the sessionLockedUntilUtc value for each session, this might not be accurate data. Apparently, if you try to settle a message before session expiry, Service Bus automatically renews the lock on the session. When this happens, the data structure for receiver on our side has no way of updating the sessionLockedUntilUtc

Oh, the second issue seems new, good to know!
I was also thinking about what the consequences of losing session lock would actually mean to the end user.
Both theoretically and practically, it doesn't seem to be related to messages or settling of messages.
I would expect session lock to affect interactions with the session. So while it makes sense the session gets closed and is inaccessible once it expires (and hence settling fails), I would expect tests to center around testing that criteria (closing of session link?) than purely on settling of messages.
Would like to see more user stories/close to real world scenarios on what losing session lock would mean, maybe a look into how other SDks have it.
(Technically, settling should succeed if attempted via new session. That makes me rethink a little about our renaming of acceptSession to getSessionReceiver)

Update - with https://github.com/Azure/azure-sdk-for-js/pull/1128:

SessionLockLostError is surfaced on both Streaming and Batching receivers. The difference being, in case of Streaming - the error from Service Bus is surfaced to the user, whereas in Batching a new error is constructed and thrown with same name.
Testing done:

streamingReceiverSessions.spec.ts and sessionTests.spec.ts pass 100%
currently running all tests except for renewLock and renewLockSessions, most of them are passing so far.
verified that SessionLockLostError is being surfaced reliably, however will continue deep-diving and thoroughly testing the changes.

Pending items:

Session Lock renewal tests will need to be updated accordingly.
Continued deep-dive on credits and credit_window

credit_window is being passed as 0, but it's not clear if that truly disables the credit management
the error does not get triggered if we add credit at the beginning of onSessionMessage handler. Could be related to - https://github.com/amqp/rhea-promise/blob/c82884b80852619d8ec31e976127a01174d412db/lib/util/utils.ts#L179

Error handling in SDK can be uniformized i.e., we could add an error constructor utility that stores names and descriptions of all error messages that we surface to user. This helps with:

better structure, review the error messages that we expose
not create random errors on the fly
serve as documentation too on all errors that can be thrown

Error handling in streaming receiver - onError can be invoked asynchronously and does not stop execution of onMessage. Wonder if this has any side-effects that user may need to be aware of as such.

credit_window is being passed as 0, but it's not clear if that truly disables the credit management

Looks like it does disable any incoming messages until addCredit or setCreditWindow is called.

Error handling in SDK can be uniformized

Yes, agreed on all counts. Logged #1145 to track this.

Error handling in streaming receiver - onError can be invoked asynchronously and does not stop execution of onMessage.

Thats an interesting scenario.
In case of error scenarios, the receiver link gets closed after notifying the user, there will be no more new messages incoming.
The interesting case is if the onMessage handler is called, and the code there is async and waiting for something, in which time the the error is notified. In such case, of the onMessage handler only user code, (i.e not interacting with the receiver) then they will complete running (as they should). If it has any code interacting with the receiver like settling a message, then that will throw an error. What happens to this error? Is the onError handler called again?

That's right, the user's onError handler would be invoked and it needs to be understood I guess. Unlike say regular promise's reject, resolve callbacks, our callbacks are not mutually exclusive. That may need to change or perhaps documented?
Yes, whenever there's an error the onError handler gets invoked one way or the other. If onMessage throws error that would stop executing.
Update - with https://github.com/Azure/azure-sdk-for-js/pull/1128:

Updated session lock renewal tests
Testing done:

All the tests pass, including lock related ones

Pending items:

Continued deep-dive on credits and credit_window

the error does not get triggered if we add credit at the beginning of onSessionMessage handler. Could be related to - https://github.com/amqp/rhea-promise/blob/c82884b80852619d8ec31e976127a01174d412db/lib/util/utils.ts#L179

Error handling in streaming receiver revisit or documentation update perhaps

Done with #1180


Add animation support

Hi Joshua Lampert,
Thanks for your template  and it is so nice for presentation in Engineering.
I would like to ask if you can add support to allow animation in touring? For instance, pause and meanwhile?
Thanks advance.

Hi @ZhongmouLi, I am glad you like it. As the template uses touying, you should already be able to use pause and meanwhile from touying. You can checkout the documentation if touying to see how you can use it. If you have any problems with that, feel free to post a minimal example.

@JoshuaLampert , Thanks for your reply.
I tired pause in the template using online typist.app, however it does not work as expected.
For instance, when I add a pause in a Focus slide and a Theorem slide, it does not generate additional slides for animation.

Hi @ZhongmouLi. I cannot reproduce this. Here is a link to a project including the #pause you inserted in the screenshot above and it works for me as expected: https://typst.app/project/rnhfaOQjwdvswTHjPttOYh. Are you sure you didn't include the handout: true option in the template (which excludes all animations from the pdf)? If the issue persists, it would be really helpful if you could provide a link to a project so I can see the complete source. Thanks!

@JoshuaLampert , thanks for your reply. It is my issue as I included #import "@preview/polylux:0.3.1": * after yours. That is the issue.  After removing it, it works as expected.
Thanks again for your efforts.

Glad to hear it works.


tap: generic body matching

Add generic text body matchers that perform matches up to some buffer limit.

@mattklein123 Since there are no assumptions about the body's structure, I think that only matchers would be:

contains token
does not contain token
where the token is string or regex.

Should the matching process kick in only when content-Type is text/plain, or leave it to a user to configure two matchers?
and_match:
  rules:
    - http_request_headers_match:
              headers:
               - name: content-type
                  exact_match: plain/text
    - generic_body_match:
             contains: envoy

@cpakulski I think that sounds about right. I probably wouldn't worry about content type and would allow binary body matching.

@mattklein123 Thank you. I am going to implement is the way you suggest.

@mattklein123 I have developed a prototype for generic body matching. As configuration it takes a list of text strings or hex strings. In order to get a match all listed strings must be found in the body. The code is on my branch: https://github.com/cpakulski/envoy/tree/issue/6107.
There are few issues I would like to discuss:

onBody method (https://github.com/cpakulski/envoy/blob/issue/6107/source/extensions/filters/http/tap/tap_config_impl.cc#L177-L215) is called from HttpPerRequestTapperImpl::onResponseBody and from HttpPerRequestTapperImpl::onRequestBody without indicating the direction. It means that the same instance of BodyMatcher is invoked for request and later on for response. Currently I implemented it using OR (https://github.com/cpakulski/envoy/blob/issue/6107/source/extensions/common/tap/tap_matcher.cc#L162) - when either direction matches the matcher indicates MATCH. There is clear direction distinction for Headers - Request and Response. I am wondering if we want to do the same for Body. There are 2 ways: 1st - use two different types GenericRequestBodyMatcher and GenericResponseBodyMatcher and 2nd way - add a direction parameter to GenericBodyMatcher api (enum : any, request, response). There are two other body matchers in the pipeline: JSON and gRPC and the same idea will have to be replicated there.

For matching I am using search method from buffer library (https://github.com/cpakulski/envoy/blob/issue/6107/source/extensions/common/tap/tap_matcher.cc#L159). It works but scans entire payload and I see no easy way to limit search to first X bytes. Are you aware of any utility already in the code where I could do text match indicating bytes limit? If not,there are 2 options: develop new text matcher or extend buffer library to limit search to indicated number of bytes.


Weird Portable Terminal interactions in a few scenarios

Describe the bug
Yeah so in a few strangely specific scenarios, it seems like whatever patches you have to fix (or something like that) the portable terminal animation don't work properly.
To Reproduce
Steps to reproduce the behavior:
Download either
018d5e13-d0ca-fbef-147c-99a1b6ccd210 (small and concise, breaks when LethalExpansion is enabled)
018d5e1f-8f05-d182-10d7-5d654faba7b4 (more of a full pack, breaks in the same way, but only when broiiler's emotes is enabled)
Attempt to open the terminal
In both of these, it breaks with the specified mod turned on, or MoreEmotes. Disabling the specified mod or MoreEmotes fixes the issue
Expected behavior
Portable Terminal should pull up properly
Screenshots

Additional context
from my perspective, it seems like in both of these cases. Whatever patch you have for MoreEmotes is failing to apply correctly, but at the same time I do not see any errors in the log indicating such.
Logfiles
Both of these logs are from the first modpack code listed above
BepinExLog LogOutput.log
Player.log
Add a list of all of your mods
For code 1:
BepInEx-BepInExPack-5.4.2100
Rune580-LethalCompany_InputUtils-0.6.1
PotatoePet-AdvancedCompany-1.0.145
Ozone-Runtime_Netcode_Patcher-0.2.5
Sligili-More_Emotes-1.3.3
HolographicWings-LethalExpansion-1.3.25
For code 2: (note that not all of these are enabled)
BepInEx-BepInExPack-5.4.2100
broiiler-broiilers_silly_emotes-1.1.7
2018-LC_API-3.4.5 --------- DISABLED
RugbugRedfern-Skinwalkers-4.0.1
FlipMods-TooManyEmotes-1.8.6
Magic_Wesley-Acidir-2.0.0
Magic_Wesley-Etern-1.3.4
Keyble-Nerd_Scraps-1.1.4
Rune580-LethalCompany_InputUtils-0.6.1
Evaisa-LethalLib-0.14.2
Insomniax-InsomniaxCosmetics-1.2.0
FlipMods-ReservedItemSlotCore-1.8.17 --------- DISABLED
FlipMods-ReservedFlashlightSlot-1.6.3 --------- DISABLED
HomelessGinger-MaskedEnemyOverhaul-3.1.1
HolographicWings-LethalExpansion-1.3.25
FlipMods-ReservedWalkieSlot-1.6.2 --------- DISABLED
EliteMasterEric-WackyCosmetics-2.0.0
Caigan-Tokucade_Scrap-1.5.0
IAmBatby-LethalLevelLoader-1.0.7
Sligili-More_Emotes-1.3.3
Evaisa-LethalThings-0.9.4
jockie-LethalExpansionCore-1.3.15
sunnobunno-BonkHitSFX-1.0.5
Mhz-MoreHead-1.2.5
Pooble-LCBetterSaves-1.7.3
Zingar-Atlas_Abyss-1.1.6
Taker-CannabisCompany-1.0.3
ViViKo-ItemClippingFix-1.1.3
Jamil-Corporate_Restructure-1.0.6
x753-Mimics-2.3.2
notnotnotswipez-MoreCompany-1.7.6
Hexnet111-SuitSaver-1.1.4
Zingar-SecretLabs-3.3.4
anormaltwig-LateCompany-1.0.10 --------- DISABLED
quackandcheese-MirrorDecor-1.3.1
PC-FireExitFlip-1.0.0
Nuts-LethalPlushies-1.7.1
Ozone-Runtime_Netcode_Patcher-0.2.5
FlipMods-FasterItemDropship-1.2.1
Azim-LethalPresents-1.0.4
Jordo-NeedyCats-1.1.1
Verity-TooManySuits-1.0.5
xboxcontroller-LCThirdPerson-1.0.4 --------- DISABLED
BACONGUDEN-VileValuables-1.0.2
akechii-DiscountAlert-2.3.0
EliteMasterEric-Coroner-1.5.3
FowD8-A_Few_More_Suits-1.5.1
Blorb-WeatherMultipliers-1.1.0 --------- DISABLED
pineapple3333-Scavenger_Plushie-1.0.3 --------- DISABLED
FlipMods-BetterStamina-1.3.2
llpaull-MineMineMineMod-1.1.0
ChooMcoo-MoreAdditionalSuits-1.0.2
Sligili-HDLethalCompany-1.5.6
OnePeak-Low_Budget_One_Piece_suits-0.5.1
PuffyMods-Kick_Me-1.1.0
ViViKo-AllTheScrap-1.0.1 --------- DISABLED
PuffyMods-Dunce_Cap-1.1.0
quackandcheese-ToggleMute-1.3.0
x753-More_Suits-1.4.1
RickArg-Helmet_Cameras-2.1.5
matsuura-HealthMetrics-1.0.2
Sv_Matt-HideModList-1.0.1
Midge-PushCompany-1.2.0
Monkeytype-HideChat-1.0.0
FlipMods-MoreBlood-1.0.2
FlipMods-SkipToMultiplayerMenu-1.0.0 --------- DISABLED
Evaisa-HookGenPatcher-0.0.5
FlipMods-LetMeLookDown-1.0.1
tinyhoot-ShipLoot-1.0.0
Insomniax-JaximuuhhsCaliicorppaModPack-1.4.2 --------- DISABLED
AinaVT-LethalConfig-1.3.4
Gemumoddo-BadAssCompany-1.1.4
Gemumoddo-LethalEmotesAPI-1.4.0
Sir_BananaCat-ToothlessDanceEmote-1.0.4
HGG-JigglePhysicsPlugin-1.1.2
mertvybasein-NekiyManEmotes-1.1.1
AnythingGoesGit-MoreDeathNotes-1.4.0 --------- DISABLED
willis81808-LethalSettings-1.3.0
Norman-Lightsabers-1.1.2
bizzlemip-BiggerLobby-2.6.0 --------- DISABLED
PotatoePet-AdvancedCompany-1.0.145
CHILLAX-ChillaxSCRAPS-0.6.0
NutNutty-SellTracker-1.2.1
mfwmt-SanrioSuits-1.3.1
CTMods-KarmaForBeingAnnoying-1.1.0

Just updating this, was doing a bit more testing and with the same mod list from the tiny profile, if UnityExplorer is added, everything works as intended. https://youtu.be/tCiTXNelcL4 (youtube since it's too big of a file to upload to github)

Should be fixed in 1.0.147. I will close this but we can reopen it if it persists.

Reopen for now as I got a report this still happens.

OK after reevaluation it seems to be fixed :D


extends nitmd to handle manpages output format

A lot of things are done here:

extension of nitmd to handle more output formats
new man decorator to transform markdown into basic groff suitable for manpages. Close #1506
remove dubious usage of definition list in our markdown. Close #1594
remove the use of pandoc and use nitmd to generate our manpages. Close #824

:shipit:

+1


Buttons in UITableView stop scrolling

After applying the emptyDataSetSource and emptyDataSetDelegate to my UITableView, all the buttons inside cells are stopping the table view to scroll.
Can reproduce by the subclassing the table view:

Subclass MyTableView : UITableView (and change the custom class in Storyboard to MyTableView)
override touchesShouldBegin:withEvent:inContentView: (return true) and make a break point inside
Add cells with UIButton to the table view
Run in Simulator and start to scroll the table view by touching (not tapping) on the button inside the cell
the touchesShouldBegin:withEvent:inContentView: would not fire
remove the reference to DZNEmptyDataSet and run again
the break point will be hit

Awesome. Thanks.


Stable

Tengine 1.5.2

 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.0 out of 4 committers have signed the CLA.:x: yzprofile:x: yaoweibin:x: weiyue:x: Chuanwen Chenweiyue, Chuanwen Chen seem not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.


Remove for loop in computing mantissa double value

Related commit in apple/swift#8804

@swift-ci please test

@swift-ci please test

looks great to me! thanks again for cross posting


Feat/simplify dockerfile

Before submitting this PR, please make sure:

[ ] Your code builds clean without any errors or warnings
[ ] You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:
Improve the installation process by removing all environment.yml files that are platform specific. Also, the Docker workflow is simplified to one file for all architectures.
The final structure has only 1 environment.yml file under the folder setup/ and 1 Dockerfile.
install.sh was modified too, and now is using the same file.
If you want to build the image:
docker build -t hummingbot/hummingbot:multi_arch_test .
In case that you want to build and push the results you can use buildx. To create a new instance
docker buildx create --platform linux/arm64,linux/amd64
docker build use $name_of_buildx_instance
docker buildx build --platform linux/arm64,linux/amd64 -t hummingbot/hummingbot:multi_arch_test --push .
That will upload a version for all architectures directly. Probably this is a good task for @david-hummingbot and @rapcmia to see how it works. Based on ChatGPT this are all the available platforms:

linux/amd64
linux/arm64
linux/arm/v7
linux/ppc64le
linux/s390x
windows/amd64
windows/arm64

Please check if that's true, to create the corresponding buildx that works for all platforms.
Here you have a refactored docker-compose.yml that works with the new dockerfile: https://gist.github.com/cardosofede/8cd795b26d2989cad376ba231b506955
Check that now we need to specify the full path: /home/hummingbot/data....
Tests performed by the developer:

uninstall and install conda environment
build docker image and run it
push image, clone it again and run it

Tips for QA testing:

Run ./uninstall and ./install in all OS (windows, ubuntu, Mac) in all arch(amd64, arm64,..)
Build docker image and run it from all combinations mentioned before
Build an image with buildx that works in all architectures and upload it to hummingbot registry
Run the compose file with the new image created by QA in all combinations of OS and Arch mentioned before

Awesome work @cardosofede !!
Beyond multi-arch support, the miniconda base image is optimized for both size and performance!

@klpanagi I removed the volumes that where created after the mount. What do you think about this, and which in use cases do you think can be better to have it?

I would suggest to rather use this environment: https://github.com/hummingbot/hummingbot/pull/6199/files#diff-13e9c25e4720c461f255e78eb529ddf7a99cc58f524bf70a7e65e637d2d760ff

It seems still valuable to have environment tuned to architectures, in some rare cases some conda packages cannot be found for an architecture and needs to be replaced by a pip
If you look at the PR I proposed you will see that most of the pip packages have conda available and should rather use conda to create a consistent environment. The PR I propose is a first step toward migrating to python 3.9 to python 3.11 and tuning the environment to have non-conflicting dependencies for each architecture
Ideally, there would be an additional script that would evaluate whether the pip packages have equivalent conda replacement and clean-up the environment
It is a bit unfortunate that we end-up having different solutions for intertwined problems :/

PR update:

Run uninstall, clean and install, all ok 💯

WSL ubuntu 20.04 ✅

Create local docker image using docker build -t hummingbot:hummingbot:$tag .

Ran buildx to test dockerhub repository

docker buildx create --name test-6229-01 --platform linux/arm64,linux/amd64 ✅
docker buildx use test-6229-01 ✅
docker buildx build --platform linux/arm64,linux/amd64 -t hummingbot/hummingbot:multi_arch_test --push . ✅
Successfully uploaded → https://hub.docker.com/repository/docker/rapcmia/test01/tags?page=1&ordering=last_updated

MBP(m1) running on anaconda3/osx-64 ✅

Successfully build local docker image

Ran tests on using helper scripts

Helper scripts working found on deploy-examples/bash-scripts/ , all ok
Create docker using local and uploaded image ✅

MBP (m1)

WSL ubuntu20.04

Most of the tests are for Client installation and still looking for other possible cases. We should also keep in mind that the approach on uploading a image to dockerhub using buildx took around 40mins

Pending:

Updating version
Gateway
Docker compose
Observe client + gateway when setting up with strategies and longterm

PR update:

Test updating version using uploaded docker image built using buildx, all ok
Docker compose simple and multiple bots ✅

@MementoRC I just saw your comments. I discussed it with @fengtality and this solution will simplify the workflow and the extra dependencies are not so heavy for the user.
The proposal for you is to create a new environment.yml with the upgrade for python 3.9-3.11 and have those 2 environment files and the user can select with python version he wants to use. What do you think?

Sure, I already have a 3.9 conda focused, but it requires some clean-up of the code (I thought there was already a PR for that)
I am supportive of the improvement. I do not understand what is wrong with the environment.yml that I proposed. It only makes the conda environment more consistent.

@MementoRC you mean the dependencies with >= ...?
I like that but at some point how we can make sure that the new version of those dependencies doesn't have a new api?
Also, I think that you removed some dependencies that are not needed and that is great too.
Once this PR is merged (that is more focussed on simplifying Docker) submit your PR. We should assign a bounty @carlitogetaladajr to memento for his work

PR update:

Test latest commit fb86656a97de0f
Check behavior of multiarch approach on test docker repo when uploading a new image

Successfully uploaded and append the old image 💯

Setup gateway docker

Run gateway generate-certs, ok on client
Check the folder, no gateway files created
Tried to change permission to sudo chmod -R a+rw  and generate certs again, still no go
Build a local image and upload to test dockerhub repo

Build docker client and generate gateway, still no go ❌

Crosscheck the development branch

Pull latest docker development image
Generate gateway certs and confirm the files are available ✅

Tried to create image using docker build -t hummingbot/hummingbot: $TAG -f Dockerfile . to check if this works using the Dockerfile

On the screenshot above seems successful yet when checking docker images it is not available. Image name was pr6229b

Test with strategies and connectors

Connect binance API key successfuly on client
Check folder, no files under conf/connectors
This is the same scenario for strategies too

It is also observed that .password_verification is not available. On the screenshot below is sample from development branch which is not the same on this PR

Test hummingbot-update.sh

Confirmed able to select the correct architecture if its AMD64 or ARM64 ✅
However it does not carry over the password, connectors and strategies. When starting the updated container, it starts from scratch ❌

Steps to reproduce:

Build local image ( optional: then upload to test docker repository )
Use the bash_scripts to create hummingbot docker client
Observed behavior after successfully connected a exchange and created strategy

Check folder directory where the docker container files are stored e.g hummingbot_files/conf/..

Observed behavior when setting up docker gateway
Observe behavior when successfully run hummingbot-update.sh

For testing uploaded multiarch you may use our test repository: https://hub.docker.com/repository/docker/rapcmia/test01/tags?page=1&ordering=last_updated

Tried to change permission to sudo chmod -R a+rw  and generate certs again, still no go

Issue related to missing files on hummingbot_files/conf and /certs folder has been fixed on this PR commit https://github.com/hummingbot/deploy-examples/pull/14/commits/85ebeda280e6c03b97a8e19d5ede8634e22f27a3 ✅
Test in progress


Docs - mention github webhook should be in json format

Docs here should mention that the content type should be json format.

Hi @narcis-ro :)
Would you be willing to add this (And other great suggestions you had) as PRs? which I then can review and approve - it would speed up the process. :)

is this issue still open ?

is this issue still open ?

It seems so - wanna take a stab at it ?


Not all nested fields are objects

e.g:
elf.header.abiversion is not a field under a header object.
{
  "elf": {
     "header" : {
        "abiversion" : ""
     }
  }
}

But a field with dots under the elf object.
{
  "elf": {
     "header.abiversion" : ""
  }
}

https://github.com/elastic/ecs-typescript/blob/main/generated/process.ts#L106-L110
See e.g how beats maps this:
https://github.com/elastic/beats/blob/66755e6b58338fd138374314c71d5d0119d1506f/libbeat/ecs/elf.go#L54-L55
And your neighbor ecs-dotnet :wave:
https://github.com/elastic/ecs-dotnet/blob/main/src/Elastic.CommonSchema/FieldSets.Generated.cs#L948-L954
Only cases where the immediate root of the field is mapped as object or nested should the typemappings create an intermediate inline object type see e.g (dns.answers](https://www.elastic.co/guide/en/ecs/master/ecs-dns.html#field-dns-answers) or elf.sections
And again how beats maps it:
https://github.com/elastic/beats/blob/66755e6b58338fd138374314c71d5d0119d1506f/libbeat/ecs/elf.go#L66
and ecs-dotnet:
https://github.com/elastic/ecs-dotnet/blob/main/src/Elastic.CommonSchema/FieldSets.Generated.cs#L1036-L1043
Currently ecs-typescript does not discover this type yet and reverts to Record<string, object>
https://github.com/elastic/ecs-typescript/blob/main/generated/elf.ts#L87

@Mpdreamz thank you for this report, I will look into it!