Hmm, clearly I broke something fundamental in https://github.com/envoyproxy/envoy/blob/b0c33ac4a58e65e22fcac0426fe12800dd4a8564/test/extensions/dynamic_modules/dynamic_modules_test.cc but I don't understand it. If anyone can offer pointers...

Well, it passed in CI this time, which is strange. Still fails locally (macOS)...

Okay, thanks, that makes sense to me. I will try it out.

Well, one thing I’m unsure of: when using IDs for stats, how can that result in a different outcome if IDs are stored past their valid lifetimes in the module? An ID could point past the end of the vector, or the vector could have changed such that an ID points at a different metric than it originally did.

IDs can be made invalid just like pointers can, right? So what is the benefit? Whether we use IDs or pointers, we have to get the rust lifetimes correct either way, don’t we?

I'm sorry for these tedious questions, but my experience with both C++ and Rust is minimal 🥲.

ok i was thinking we don't need to think about that case. ID is passed together with envoy_dynamic_module_type_http_filter_envoy_ptr which holds the pointer to the underlying Envoy's FIlterConfig object that eventually holds vector. It should be no issue as long as modules are not intentionally passing invalid IDs together with a wrong envoy_dynamic_module_type_http_filter_config_envoy_ptr? The basic assumption of dynamic modules is modules must be trusted. so we don't need to care about these "misbehaving" modules. In fact, we have plenty of codes that can crash envoy if the modules are malicious since modules have the same privilege as any other native filters.

That is totally different than the fact that not exposing raw pointer, like the current impl, completely forbids the Rust code from writing the crash since the "updates" ABI binds the lifetime of the underlying stats objects if you require the ABI callbacks to pass "filter pointers" vs not as in the current impl.

the vector could have changed such that an ID points at a different metric than it originally did.

also this doesn't happen as I described in the review comment ^^. Let me repeat: "define ABIs" are only available during filter config initialization that happens only once before any stream handling. That is why I suggested the use of vector and it's immutable by the time when "updates" are available, which doesn't require lock or whatever.

On second thought; using the raw pointer address as an ID would be fine as long as the "updates" callbacks are bounded within the lifecycles of filter_config as per the suggestion. That way, we won't need to maintain vector at all. e

@mathetake I think I've addressed the easy parts of your feedback, but I am struggling to prevent using Envoy{Counter,Gauge,Histogram}s in the Rust SDK that wrap invalid pointers (due to the filter config being deallocated). I think the first step is to have these structs stop implementing Copy/Clone, but I am not sure what to do after that.

I have tried annotating many things with lifetimes, trying to make these structs have the same lifetime as EnvoyHttpFilterConfig, but it requires annotating tons of structs, traits, and functions (it'd be a big breaking change to the SDK), and I still can't make it compile in the end. Do you think it's possible to make it work this way, or should I be trying to do something else?

@isker @mathetake Prototyped something like this to keep the Rust references bounded by the lifetime of the filter config and filter: wtzhang23@02f32ee

Let me know if this helps :)

@isker @mathetake Prototyped something like this to keep the Rust references bounded by the lifetime of the filter config and filter: wtzhang23@02f32ee

Let me know if this helps :)

Thanks @wtzhang23, and the idea looks great but I would rather against exposing the entire EnvoyHttpFilterConfig to per-request struct since that would mean exposing the mutable non-thread safe operations (define_* metrics ABI) on to worker threads vs only exposing the safe operations (increments).

My suggestion is simple:

• Delete pointer types as per previous comments
• Couple all "updates" callbacks (set, record, increment, decrement) with filter_envoy_ptr, not filter_config_envoy_ptr. It is guaranteed that filter_config_envoy_ptr outlives filter_envoy_ptr on envoy side. So it would be always safe as long as users are sane.
  • This is why I would add envoy_dynamic_module_callback_http_filter_metric_ prefix for updates callbacks vs envoy_dynamic_module_callback_http_filter_config_metric_ for define callbacks

This will prevent at least the invalid pointers crash on the misuse due to the lifetime issue. The only remaining problem is when user passing IDs to the filter not created from the filter config that didn't create the metric/id. I think that would be a negligible as that means the "insane" ID passing happening via global shared memory. Even it that happens, Envoy won't crash by boundary check vs previously in the initial iteration of this PR, one could simply store the metrics pointer in the global cache (which I believe one could do that without thinking too much).

wdyt? I think there won't be a perfect solution at the point. so maybe the question is how much we could reduce the opportunity for module devs to write wrong code. I am 100% open to other ideas.

diff --git a/source/extensions/dynamic_modules/abi.h b/source/extensions/dynamic_modules/abi.h
index 601e7ff0c5..f3b70f03db 100644
--- a/source/extensions/dynamic_modules/abi.h
+++ b/source/extensions/dynamic_modules/abi.h
@@ -124,33 +124,6 @@ typedef void* envoy_dynamic_module_type_http_filter_envoy_ptr;
  */
 typedef const void* envoy_dynamic_module_type_http_filter_module_ptr;
 
-/**
- * envoy_dynamic_module_type_metric_counter_envoy_ptr is a raw pointer to the Stats::Counter class
- * in Envoy. This is passed to the module when creating a new Counter during filter initialization.
- * It can be used to manipulate the counter through various callbacks into Envoy.
- *
- * OWNERSHIP: Envoy owns the pointer.
- */
-typedef void* envoy_dynamic_module_type_metric_counter_envoy_ptr;
-
-/**
- * envoy_dynamic_module_type_metric_gauge_envoy_ptr is a raw pointer to the Stats::Gauge class in
- * Envoy. This is passed to the module when creating a new Gauge during filter initialization. It
- * can be used to manipulate the gauge through various callbacks into Envoy.
- *
- * OWNERSHIP: Envoy owns the pointer.
- */
-typedef void* envoy_dynamic_module_type_metric_gauge_envoy_ptr;
-
-/**
- * envoy_dynamic_module_type_metric_histogram_envoy_ptr is a raw pointer to the Stats::Histogram
- * class in Envoy. This is passed to the module when creating a new Histogram during filter
- * initialization. It can be used to manipulate the histogram through various callbacks into Envoy.
- *
- * OWNERSHIP: Envoy owns the pointer.
- */
-typedef void* envoy_dynamic_module_type_metric_histogram_envoy_ptr;
-
 /**
  * envoy_dynamic_module_type_http_filter_scheduler_ptr is a raw pointer to the
  * DynamicModuleHttpFilterScheduler class in Envoy.
@@ -804,24 +777,25 @@ bool envoy_dynamic_module_callback_log_enabled(envoy_dynamic_module_type_log_lev
  * counter will be defined.
  * @param name is the name of the counter to be defined.
  * @param name_length is the length of the name.
- * @return a pointer to the defined counter that may be used to increment it later. This pointer is
- * valid only as long as filter_config_envoy_ptr is valid.
+ * @return an opaque ID that represents a unique metric. This can be passed to
+ * envoy_dynamic_module_callback_metric_increment_counter together with filter_envoy_ptr created
+ * from filter_config_envoy_ptr.

...skipping 1 line
-envoy_dynamic_module_type_metric_counter_envoy_ptr
-envoy_dynamic_module_callback_metric_define_counter(
+size_t envoy_dynamic_module_callback_http_filter_config_metric_define_counter(
     envoy_dynamic_module_type_http_filter_config_envoy_ptr filter_config_envoy_ptr,
     envoy_dynamic_module_type_buffer_module_ptr name, size_t name_length);
 
 /**
- * envoy_dynamic_module_callback_metric_increment_counter is called by the module to increment a
+ * envoy_dynamic_module_callback_http_filter_metric_increment_counter is called by the module to increment a
  * previously defined counter.
  *
- * @param counter_envoy_ptr is a pointer to a counter previously defined using
- * envoy_dynamic_module_callback_metric_define_counter.
+ * @param filter_envoy_ptr is the pointer to the DynamicModuleHttpFilter object.
+ * @param id is the ID of the counter previously defined using the config that created
+ * filter_envoy_ptr.
  * @param value is the value to increment the counter by.
  */
-void envoy_dynamic_module_callback_metric_increment_counter(
-    envoy_dynamic_module_type_metric_counter_envoy_ptr counter_envoy_ptr, uint64_t value);
+void envoy_dynamic_module_callback_http_filter_metric_increment_counter(
+    envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, size_t id, uint64_t value);
 
 /**
  * envoy_dynamic_module_callback_metric_define_gauge is called by the module during initialization
@@ -831,45 +805,53 @@ void envoy_dynamic_module_callback_metric_increment_counter(
  * gauge will be defined.
  * @param name is the name of the gauge to be defined.
  * @param name_length is the length of the name.
- * @return a pointer to the defined gauge that may be used to manipulate it later. This pointer is
- * valid only as long as filter_config_envoy_ptr is valid.
+ * @return an opaque ID that represents a unique metric. This can be passed to
+ * envoy_dynamic_module_callback_metric_increment_counter together with filter_envoy_ptr created
+ * from filter_config_envoy_ptr.
  */
-envoy_dynamic_module_type_metric_gauge_envoy_ptr envoy_dynamic_module_callback_metric_define_gauge(
+size_t envoy_dynamic_module_callback_http_filter_config_metric_define_gauge(
     envoy_dynamic_module_type_http_filter_config_envoy_ptr filter_config_envoy_ptr,
     envoy_dynamic_module_type_buffer_module_ptr name, size_t name_length);
 
 /**
- * envoy_dynamic_module_callback_metric_increase_gauge is called by the module to increase the value
+ * envoy_dynamic_module_callback_http_filter_metric_increase_gauge is called by the module to
+ * increase the value
  * of a previously defined gauge.
  *
- * @param gauge_envoy_ptr is a pointer to a gauge previously defined using

...skipping 1 line
+ * @param filter_envoy_ptr is the pointer to the DynamicModuleHttpFilter object.
+ * @param id is the ID of the gauge previously defined using the filter config that created
+ * the filter_envoy_ptr.
  * @param value is the value to increase the gauge by.
  */
-void envoy_dynamic_module_callback_metric_increase_gauge(
-    envoy_dynamic_module_type_metric_gauge_envoy_ptr gauge_envoy_ptr, uint64_t value);
+void envoy_dynamic_module_callback_http_filter_metric_increase_gauge(
+    envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, size_t id, uint64_t value);
 
 /**
- * envoy_dynamic_module_callback_metric_decrease_gauge is called by the module to decrease the value
+ * envoy_dynamic_module_callback_http_filter_metric_decrease_gauge is called by the module to decrease the value
  * of a previously defined gauge.
  *
- * @param gauge_envoy_ptr is a pointer to a gauge previously defined using
+ * @param filter_envoy_ptr is the pointer to the DynamicModuleHttpFilter object.
+ * @param id is the ID of the gauge previously defined using the filter config that created
+ * the filter_envoy_ptr.
  * envoy_dynamic_module_callback_metric_define_gauge.
  * @param value is the value to decrease the gauge by.
  */
-void envoy_dynamic_module_callback_metric_decrease_gauge(
-    envoy_dynamic_module_type_metric_gauge_envoy_ptr gauge_envoy_ptr, uint64_t value);
+void envoy_dynamic_module_callback_http_filter_metric_decrease_gauge(
+    envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, size_t id, uint64_t value);
 
 /**
  * envoy_dynamic_module_callback_metric_decrease_gauge is called by the module to set the value
  * of a previously defined gauge.
  *
- * @param gauge_envoy_ptr is a pointer to a gauge previously defined using
+ * @param filter_envoy_ptr is the pointer to the DynamicModuleHttpFilter object.
+ * @param id is the ID of the gauge previously defined using the filter config that created
+ * the filter_envoy_ptr.
  * envoy_dynamic_module_callback_metric_define_gauge.
  * @param value is the value to set the gauge to.
  */
-void envoy_dynamic_module_callback_metric_set_gauge(
-    envoy_dynamic_module_type_metric_gauge_envoy_ptr gauge_envoy_ptr, uint64_t value);
+void envoy_dynamic_module_callback_http_filter_metric_set_gauge(
+    envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, size_t gauge_envoy_ptr,
+    uint64_t value);
 
 /**
  * envoy_dynamic_module_callback_metric_define_histogram is called by the module during
@@ -879,24 +861,26 @@ void envoy_dynamic_module_callback_metric_set_gauge(

...skipping 1 line
  * @param name is the name of the histogram to be defined.
  * @param name_length is the length of the name.
- * @return a pointer to the defined histogram that may be used to record values to it later. This
- * pointer is valid only as long as filter_config_envoy_ptr is valid.
+ * @return an opaque ID that represents a unique metric. This can be passed to
+ * envoy_dynamic_module_callback_metric_increment_counter together with filter_envoy_ptr created
+ * from filter_config_envoy_ptr.
  */
-envoy_dynamic_module_type_metric_histogram_envoy_ptr
-envoy_dynamic_module_callback_metric_define_histogram(
+size_t envoy_dynamic_module_callback_http_filter_config_metric_define_histogram(
     envoy_dynamic_module_type_http_filter_config_envoy_ptr filter_config_envoy_ptr,
     envoy_dynamic_module_type_buffer_module_ptr name, size_t name_length);
 
 /**
- * envoy_dynamic_module_callback_metric_record_histogram_value is called by the module to record a
+ * envoy_dynamic_module_callback_http_filter_metric_record_histogram_value is called by the module to record a
  * value in a previously defined histogram.
  *
- * @param histogram_envoy_ptr is a pointer to a histogram previously defined using
+ * @param filter_envoy_ptr is the pointer to the DynamicModuleHttpFilter object.
+ * @param id is the ID of the gauge previously defined using the filter config that created
+ * the filter_envoy_ptr.
  * envoy_dynamic_module_callback_metric_define_histogram.
  * @param value is the value to record in the histogram.
  */
-void envoy_dynamic_module_callback_metric_record_histogram_value(
-    envoy_dynamic_module_type_metric_histogram_envoy_ptr histogram_envoy_ptr, uint64_t value);
+void envoy_dynamic_module_callback_http_filter_metric_record_histogram_value(
+    envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, size_t id, uint64_t value);

/wait

Not sure if it is possible in standard Envoy filters but if it is, I can see a use case where someone would want to update a metric on the envoy_dynamic_module_on_http_filter_config_new and envoy_dynamic_module_on_http_filter_config_destroy callbacks. For example for showing current module versions in metrics to detect drift as modules are being updated across a cluster of Envoys.

Like a metric module_active{version="0.1.0"} 1 in prometheus format.

Yeah we can add them two in addition to the callbacks above

Won't the ABI as written in wtzhang23@02f32ee still allow null pointers to be dereferenced when you use an index into the vector that's out of bounds?

Oh, maybe not with @mathetake's patch. Okay. I will try to cherry pick this all together.

Another alternative API where you still get a reference to counter, gauge, and histogram when declaring metrics: wtzhang23@b07c1f0. Also includes fixes to the existing test cases (though not full coverage), fixes to the Rust HttpFilterConfig trait (probably should be a separate PR), some method renaming, and passing the filter config pointer to the envoy_dynamic_module_on_http_filter_config_destroy callback.

So, addressing

Couple all "updates" callbacks (set, record, increment, decrement) with filter_envoy_ptr, not filter_config_envoy_ptr. It is guaranteed that filter_config_envoy_ptr outlives filter_envoy_ptr on envoy side. So it would be always safe as long as users are sane.
This is why I would add envoy_dynamic_module_callback_http_filter_metric_ prefix for updates callbacks vs envoy_dynamic_module_callback_http_filter_config_metric_ for define callbacks

But not

Delete pointer types as per previous comments

I don't have a strong opinion on using ids vs first exchanging the ids with a pointer then using the pointer directly. I think both will end up safe. Tradeoffs off the top of my head:

Pros with using pointer:

• metric functions such as increment, decrement, ... are shared regardless of the location of the entrypoint. So, all you would need to do at every location is implement the function for exchanging the id for a pointer with lifetime bounded by the "context" of the callback. Though to be fair, the number of locations is probably minimal.
• Might be a little more forward compatible and less maintenance burden as the locations grow or when we support dynamic metric names (e.g. for metrics with labels) in the future?

Pros with defining callbacks that take the "context" and id as arguments:

• Only requires a single callback to get a metric for an id and then use it so:
  • more ergonomic in C since you don't have to chain two functions to increment a counter
  • more optimized

There's also the third approach of mixing the two approaches and having define_* return ids while also defining callbacks to exchange them for metric pointers. This is probably better than having define_* return metric pointers directly since we can then get rid of the metric->get_id callbacks.

thanks, but I would prefer the simpler C interface as well as less methods/struct at Rust layer compared to my suggestion plus your revised version still didn't address my comment about per-request object being able to define metrics concurrently.

Let's go with my suggestion for now, and shall we see what improvement can be made based on your effort to unblock the feature at least in a follow up if necessary? We can break ABI at any point as we don't guarantee the compatibility across versions.

For tags specifically, we should study how Istio does for their stats extensions (in native C++) as per my old comment, but we can do that in a follow up. Let's get the basic stats functionality working first

note2: tags handling, we might want to investigate how other filters are dealing with. We might want to study some ideas how Istio's stats filters are dealing with it. They are adding "per-request" attributes as tags. https://github.com/istio/proxy/blob/131a090f997edab4e4dc59f05ef265d895f7885e/source/extensions/filters/http/istio_stats/istio_stats.cc#L376

@isker I think you should be able to just cherry-pick this commit: wtzhang23@502299c. Simplified trying to follow @mathetake's recommended interface. Sorry in advance if I messed up the consolidation and juggling of changes.

Edit: wtzhang23@f6a26fa without the whitespace changes

I have cherry picked @wtzhang23's commit and added a fix to test compilation, which was failing. That fix reveals that, with this design:

Couple all "updates" callbacks (set, record, increment, decrement) with filter_envoy_ptr, not filter_config_envoy_ptr. It is guaranteed that filter_config_envoy_ptr outlives filter_envoy_ptr on envoy side. So it would be always safe as long as users are sane.

It is not possible to increment counters in new_http_filter in the rust SDK, because that fn is not called with EnvoyHttpFilter, only EnvoyHttpFilterConfig. We could change the signature to include EnvoyHttpFilter, because it is in the ABI signature in envoy_dynamic_module_on_http_filter_new, but I'd guess that @mathetake might not like that idea, because most of the other functions on EnvoyHttpFilter are not valid to call at that time.

Let me know what you think, though. I am not sure it would be a common desire to manipulate counters in new_http_filter, it's just what I previously wrote in the test case.

Nevermind what I said, I did not realize there were conflicts with #40864, which I think will fix this problem.

/wait
@mathetake waiting for your response.
For Istio stats, I'd recommend using stats eviction instead of explicit handles on stats since I don't think it's reasonable to expect module to manage lifetimes.

yeah sorry will do the review by the end of tomorrow 🙏

Thanks to you both for struggling through this with me 🫡.