IMO Otel client library is probably the better UX than prometheus at this point https://opentelemetry.io/docs/languages/go/instrumentation/

I think an api similar to otel would require both label names and values to be inserted into the per-filter instance stat name pool. In addition, if we want a simple api where the existing callbacks for defining unlabelled metrics is used to define potentially-labeled metrics, then those counters must be lazily fetched during the lifecycle of the filter. I don't think this is that big of a deal given that the metric values will be in thread-local storage after first invocation, but not 100% sure.

Not sure how expensive it is to not string intern here so not sure how big of a problem this is; at least the filters I've seen internally almost never always take the time to string intern. In this PR, I was somewhat hesitant to expose interning directly to the user, but it could be considered. Pseudocode like:

enum stat_type {
  ID,
  RAW,
}

struct str {
  char* ptr;
  size_t len;
}

struct stat_name {
  stat_type type_;
  union {
    str raw;
    size_t id;
  }
}

struct tag {
  stat_name name;
  stat_name value;
}

// returns id to represent a stat name
size_t reserve_stat_name(name);
...

void increment_counter(..., tag* tags, size_t num_tags);

AFAIK, interning requires a lock on the symbol table, so it cannot be trusted to be run on the data path. If we aspire to support SDKs, none of them really have interning capability AFAIK, and the tags are ultimately represented as vectors of strings somewhere. This is the same representation as Envoy's "inlined string", so it should perform comparatively the same.

If you really care about performance, then I think putting a bit of thought into histograms might pay the best value. They are very expensive in Envoy today, so you might need an alternate storage backend to support more rudimentary use cases (e.g. for plugins willing to take a big hit in accuracy - which is not an option today, all histograms are ~5% accurate today).

Took the opposite approach where return a result error code but pass in a pointer for the resultant metric id. This way I didn't have to think about casting size_t to int32_t (though in practice Envoy will probably crash before > than INT32_MAX metrics of the same type are created) and casting the result code + negating it into int32_t.

@wtzhang23 sorry where did you see the flake? i couldn't find here

https://github.com/envoyproxy/envoy/actions/runs/17689845108/job/50281355553

oh in the prechecks... has this been happening on main branch as well?

ok in this integration test

so FakeExternalCache this test uses the scheduler API and the new integration tests runs after it; maybe existing bug

Ya though from my staring at the code it seems your implementation should be very sound. this can't be null since the free callback can't be called while commit goes on, filter_ won't be null when upgrading to shared and calling onScheduled as checked by the if statement, dispatcher_ should live for the lifetime of the worker thread, and all the logic should be executed by the dispatcher on the thread that owns the filter should the filter be upgraded to shared... Not sure how the integration test framework works but do you think it is possible a nullptr was accidentally passed in as the argument to dispatcher here?

Edit: Oh does dispatcher really last that long? Don't know enough to see if the comment // The callbacks for the filter. They are only valid until onDestroy() is called. applies here to the lifetime of the dispatcher. Though I don't see how this would lead to a failure in the test case, but I haven't looked to carefully.

I believe dispatcher should live that long;

I have a slight hunch that this is something to do with the FakeExternalCache sending local reply from the on_scheduled callback. Could you try pushing the following patch and see if this happens again here? I think this is in anyways weird

diff --git a/source/extensions/filters/http/dynamic_modules/filter.cc b/source/extensions/filters/http/dynamic_modules/filter.cc
index 7ad53c5dca..82e99f119e 100644
--- a/source/extensions/filters/http/dynamic_modules/filter.cc
+++ b/source/extensions/filters/http/dynamic_modules/filter.cc
@@ -126,8 +126,8 @@ void DynamicModuleHttpFilter::sendLocalReply(
     Code code, absl::string_view body,
     std::function<void(ResponseHeaderMap& headers)> modify_headers,
     const absl::optional<Grpc::Status::GrpcStatus> grpc_status, absl::string_view details) {
-  decoder_callbacks_->sendLocalReply(code, body, modify_headers, grpc_status, details);
   sent_local_reply_ = true;
+  decoder_callbacks_->sendLocalReply(code, body, modify_headers, grpc_status, details);
 }

Will add tests for errors to resolve coverage failures

looks like the flake is gone? I think my hunch was right?

Ya you are a genius lol

cool lol... can you merge the main branch again so that we can be extra sure that it's gone

//test/extensions/dynamic_modules/http:integration_test is failing ASAN, PTAL? link to raw test failure logs.

oh ok thanks for the heads up. I will revert this one for now

Is this failure related to this PR? Program counter being zero is somewhat strange and the test case is not a stats test case

IpVersions/DynamicModulesIntegrationTest.PerRouteConfig/IPv4AddressSanitizer:DEADLYSIGNAL=====================================================================
ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x7f324ed9d650 sp 0x7f324ed9d468 T97)==287==Hint: pc points to the zero page.==287==The signal is caused by a READ memory access.==287==Hint: address points to the zero page./bin/addr2line: DWARF error: invalid or unhandled FORM value: 0x25

oh

Is it possible the address sanitizer just can't read the symbol table/other metadata in the module binary and it checked randomly while the program was currently in the module? Not exactly sure how address sanitizers themselves are implemented

/bin/addr2line: DWARF error: invalid or unhandled FORM value: 0x25 was also emitted in the output FWIW

#41094 opened an issue