PIP-341: Pluggable client metrics tracker interface #22145

KevinLiLu · 2024-02-28T08:13:11Z

TODO:

Should OTel implementation be included in this, or a separate follow up PIP?
Do we need to maintain compatibility with the existing metrics/stats system (ex. the public Producer/Consumer's getStats() API)
Identify additional events to record

Motivation

Modifications

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

github-actions · 2024-02-28T08:13:37Z

@KevinLiLu Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

asafm

This the first pass.
In the meeting today we can discuss in high level.

asafm · 2024-02-29T13:51:22Z

pip/pip-341.md

+
+Since we need to record events in both layers, we can create and store the metric trackers in the base classes `ProducerBase`/`ConsumerBase`, so that both `ProducerImpl`/`ConsumerImpl` and `PartitionedProducerImpl`/`MultiTopicsConsumerImpl` will have its own metric tracker.
+
+For a non-partitioned topic, there will only be one producer/consumer metrics tracker. For partitioned topics, there will be one producer/consumer tracker per partition (topic name `{partitionedTopicName}-partition-{partitionNum}`), and an additional tracker on the multi-topic/partitioned wrapper (topic name `{partitionedTopicName}`). It is up to the metric tracker implementation to decide on how to handle this (ex. aggregation at `partitionedTopicName` level).


We might need to make it easy for implementors to prepare a partition number label if it is a partition. We can have another interface targeted at that or some other means. Let's avoid topic parsing :)

We could add partitionIndex as a parameter to the MetricsTrackerFactory#create producer/consumer metrics tracker methods. Value will be -1 for non-partitioned topics. This is the same approach as Producer/ConsumerImpl creation

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

Lines 436 to 453 in 74be3fd

* @param partitionIndex partition index of a partitioned topic. the value -1 is used for non-partitioned topics.

* @param conf producer configuration

* @param schema topic schema

* @param interceptors producer interceptors

* @param producerCreatedFuture future for signaling completion of async producer creation

* @param <T> message type class

*

* @return a producer instance

*/

protected <T> ProducerImpl<T> newProducerImpl(String topic, int partitionIndex,

ProducerConfigurationData conf,

Schema<T> schema,

ProducerInterceptors interceptors,

CompletableFuture<Producer<T>> producerCreatedFuture,

Optional<String> overrideProducerName) {

return new ProducerImpl<>(PulsarClientImpl.this, topic, conf, producerCreatedFuture, partitionIndex, schema,

interceptors, overrideProducerName);

}

asafm · 2024-02-29T13:51:42Z

pip/pip-341.md

+
+    void close();
+
+    ProducerStats getStats();


Why do we need to force this new interface implementation to implement that?

asafm · 2024-02-29T13:52:37Z

pip/pip-341.md

+      try {
+          this.metricsTracker.recordMessagesSent(numMsgs, totalMsgsSize);
+      } catch (Throwable e) {
+          log.warn("Error executing producer metrics tracker recordMessagesSent call ", e);


Probably throttled.

asafm · 2024-02-29T13:54:23Z

pip/pip-341.md

+### LoggingMetricsTrackerFactory
+The default configured metrics tracker factory will be the `LoggingMetricsTrackerFactory` as it will replace the existing functionality.
+
+The logging producer/consumer metrics tracker will initialize and hold an instance of the underlying stats recorder, and forward all calls to the stats recorder.


I prefer to do that in the infrastructure and not a concrete implementation.
and after 2 versions, remove it.

asafm · 2024-02-29T13:56:22Z

pip/pip-341.md

+
+We will set the default metrics tracker factory as the `LoggingMetricsTrackerFactory` to maintain backwards compatibility, however we still need to do some checks:
+1. If a non-default non-null metrics tracker factory (ex. OTel one) is configured, then use it
+2. Else if `statsIntervalSeconds=0`, then the user has explicitly disabled client metrics so we must use the `NoOpMetricsTrackerFactory`


As said before, I wouldn't couple those two.

asafm · 2024-02-29T13:58:45Z

pip/pip-341.md

+2. Else if `statsIntervalSeconds=0`, then the user has explicitly disabled client metrics so we must use the `NoOpMetricsTrackerFactory`
+3. Else, use `LoggingMetricsTrackerFactory`
+
+Inside `LoggingMetricsTrackerFactory`, we must also derive `statsIntervalSeconds`:


I see it a bit differently.
The producerStats, consumerStats, and its matching interval config will be marked deprecated and removed in 2 subsequent versions.
Users can choose a LoggingMetricsTrackerFactory, which they configure with a new parameter, or build on their own and provide the factory instance, already configured.

asafm · 2024-02-29T14:00:20Z

pip/pip-341.md

+3. Finally, default to 60 seconds if `statsIntervalSeconds` is not configured anywhere
+
+### Supporting existing Consumer/Producer getStats API
+There are two major challenges with supporting the existing producer/consumer `getStats()` API:


If you have the two mechanisms in place, this problem disappears. This implementation will be deleted in two versions from now.

asafm · 2024-02-29T14:01:15Z

pip/pip-341.md

+1. Consolidate to a new stats supplier class to handle both partitioned & non-partitioned cases
+2. Add setter/builder/supplier methods to the new stats supplier class to allow the metrics tracker to supply stat values
+
+#### Consolidated ProducerStatsSupplier/ConsumerStatsSupplier


What's the context here. Maybe you should explain in background knowledge what are those suppliers.

dao-jun · 2024-02-29T14:44:23Z

+1 for the motivation.

PIP-341: Pluggable client metrics tracker interface

1cc560f

KevinLiLu marked this pull request as draft February 28, 2024 08:13

github-actions bot added type/PIP doc-label-missing labels Feb 28, 2024

asafm reviewed Feb 29, 2024

View reviewed changes

dao-jun assigned KevinLiLu Feb 29, 2024

dao-jun added area/metrics area/client labels Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-341: Pluggable client metrics tracker interface #22145

PIP-341: Pluggable client metrics tracker interface #22145

KevinLiLu commented Feb 28, 2024 •

edited

Loading

github-actions bot commented Feb 28, 2024

asafm left a comment

asafm Feb 29, 2024

KevinLiLu Feb 29, 2024

asafm Feb 29, 2024

asafm Feb 29, 2024

asafm Feb 29, 2024

asafm Feb 29, 2024

asafm Feb 29, 2024

asafm Feb 29, 2024

asafm Feb 29, 2024

dao-jun commented Feb 29, 2024


		Since we need to record events in both layers, we can create and store the metric trackers in the base classes `ProducerBase`/`ConsumerBase`, so that both `ProducerImpl`/`ConsumerImpl` and `PartitionedProducerImpl`/`MultiTopicsConsumerImpl` will have its own metric tracker.

		For a non-partitioned topic, there will only be one producer/consumer metrics tracker. For partitioned topics, there will be one producer/consumer tracker per partition (topic name `{partitionedTopicName}-partition-{partitionNum}`), and an additional tracker on the multi-topic/partitioned wrapper (topic name `{partitionedTopicName}`). It is up to the metric tracker implementation to decide on how to handle this (ex. aggregation at `partitionedTopicName` level).

	* @param partitionIndex partition index of a partitioned topic. the value -1 is used for non-partitioned topics.
	* @param conf producer configuration
	* @param schema topic schema
	* @param interceptors producer interceptors
	* @param producerCreatedFuture future for signaling completion of async producer creation
	* @param <T> message type class
	*
	* @return a producer instance
	*/
	protected <T> ProducerImpl<T> newProducerImpl(String topic, int partitionIndex,
	ProducerConfigurationData conf,
	Schema<T> schema,
	ProducerInterceptors interceptors,
	CompletableFuture<Producer<T>> producerCreatedFuture,
	Optional<String> overrideProducerName) {
	return new ProducerImpl<>(PulsarClientImpl.this, topic, conf, producerCreatedFuture, partitionIndex, schema,
	interceptors, overrideProducerName);
	}

PIP-341: Pluggable client metrics tracker interface #22145

Are you sure you want to change the base?

PIP-341: Pluggable client metrics tracker interface #22145

Conversation

KevinLiLu commented Feb 28, 2024 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

github-actions bot commented Feb 28, 2024

asafm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dao-jun commented Feb 29, 2024

KevinLiLu commented Feb 28, 2024 •

edited

Loading