r2dbc-proxy icon indicating copy to clipboard operation
r2dbc-proxy copied to clipboard

ObservationProxyExecutionListener + virtual threads = 💥

Open cfredri4 opened this issue 1 year ago • 2 comments

Thread name is a low cardinality key but with virtual threads there are millions of threads which leads to e.g. prometheus going 💥.

cfredri4 avatar Jul 02 '24 12:07 cfredri4

Thanks for the report.

To work around this issue, you can create a custom convention that does not record the thread name, or use an observation filter to remove the thread name tag.

Here’s an example of how you can create a custom convention:

public interface SimpleQueryObservationConvention extends QueryObservationConvention {

    @Override
    default KeyValues getLowCardinalityKeyValues(QueryContext context) {
        Set<KeyValue> keyValues = new HashSet<>();
        keyValues.add(KeyValue.of(R2dbcObservationDocumentation.LowCardinalityKeys.CONNECTION, context.getConnectionName()));
        return KeyValues.of(keyValues);
    }

}

And here’s an example of an observation filter:

public class MyObservationFilter implements ObservationFilter {

    @Override
    public Observation.Context map(Observation.Context context) {
        if (context instanceof QueryContext) {
            context.remove(R2dbcObservationDocumentation.LowCardinalityKeys.THREAD.asString());
        }
        return context;
    }
}

I will likely add this custom convention implementation to the library, allowing users to choose between the default or this custom convention.

If you are using Spring Boot, defining a convention or observation filter bean should be automatically picked up, if I remember correctly.

ttddyy avatar Jul 05 '24 21:07 ttddyy

Thank you, this is what I'm doing now. But I opened the issue as I believe the behavior should be reversed; the safest behavior that will always work would be for thread name to not be included, and then it instead can be added as an option if you know you have a low number of threads.

cfredri4 avatar Jul 06 '24 17:07 cfredri4

@ttddyy

I will likely add this custom convention implementation to the library

Is there an update on this? I just stumbled upon this myself. Both of the suggested workarounds aren't really ideal:

  • An ObservationFilter won't apply to long task timers, so I'd have to globally disable long task timers to get R2DBC observations, and even that doesn't apply to the r2dbc_query_r2dbc_query_result_total meter.
  • Overriding the QueryObservationDocumentation works, but is brittle. I'd have to keep my implementation in sync with yours. And since R2dbcObservationDocumentation.LowCardinalityKeys isn't public, I can't compile against the THREAD enum value, forcing me to repeat the "r2dbc.thread" string in my code.

I'm using this for now:

@Bean
QueryObservationConvention queryObservationConvention() {
    class RemoveR2dbcThreadObservationConvention implements QueryObservationConvention {

        @Override
        public KeyValues getLowCardinalityKeyValues(QueryContext context) {
            return QueryObservationConvention.super.getLowCardinalityKeyValues(context).stream()
                    .filter(kv -> !kv.getKey().equals("r2dbc.thread"))
                    .map(KeyValues::of)
                    .reduce(KeyValues.empty(), KeyValues::concat);
        }
    }
    return new RemoveR2dbcThreadObservationConvention();
}

kzander91 avatar Apr 24 '25 18:04 kzander91

@kzander91 Thanks for your feedback. Let me try to spend some time on this soon.

ttddyy avatar Apr 25 '25 06:04 ttddyy

I have added VirtualThreadQueryObservationConvention, which simply omits tagging the thread name.

In a Spring Boot environment, you can override the default QueryObservationConvention by creating a bean of this class. Alternatively, you can manually set this class on the ObservationProxyExecutionListener.

ttddyy avatar Apr 29 '25 00:04 ttddyy