[feat][monitor] PIP-223: Add metrics for all rest endpoints. by dao-jun · Pull Request #21772 · apache/pulsar

dao-jun · 2023-12-20T20:40:39Z

PIP: #18560

Motivation

#18560

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: dao-jun#6

dao-jun · 2023-12-20T20:44:07Z

refers to: #18836

codecov-commenter · 2024-02-29T13:27:38Z

Codecov Report

Attention: Patch coverage is 86.79245% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 73.59%. Comparing base (bbc6224) to head (e139404).
Report is 19 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #21772      +/-   ##
============================================
+ Coverage     73.57%   73.59%   +0.01%     
+ Complexity    32624    32153     -471     
============================================
  Files          1877     1878       +1     
  Lines        139502   139603     +101     
  Branches      15299    15321      +22     
============================================
+ Hits         102638   102737      +99     
+ Misses        28908    28878      -30     
- Partials       7956     7988      +32

Flag	Coverage Δ
inttests	`26.43% <3.77%> (+1.85%)`	⬆️
systests	`24.31% <3.77%> (-0.02%)`	⬇️
unittests	`72.84% <86.79%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java	`99.39% <100.00%> (+<0.01%)`	⬆️
.../java/org/apache/pulsar/broker/web/WebService.java	`94.21% <100.00%> (+0.06%)`	⬆️
...e/pulsar/broker/web/RestEndpointMetricsFilter.java	`85.71% <85.71%> (ø)`

... and 74 files with indirect coverage changes

asafm

Thanks for implementing this.
Please note that we have already started implementing PIP-264, which is Open Telemetry. We have added the infrastructure needed to define metrics, and we are underway of adding the first metrics.
Once we finish the first PR, can we ping you to add those metrics using OTel? Once that PR we fill finally know the naming conventions to use.

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java

asafm · 2024-03-03T13:53:36Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/BrokerRestEndpointMetricsTest.java

+        admin.namespaces().deleteNamespace("test/test");
+        admin.tenants().deleteTenant("test");
+
+        ByteArrayOutputStream output = new ByteArrayOutputStream();


Save yourself this code and the intimate knowledge of how it is implemented:
Create a client like this against the local broker.

prometheusMetricsClient = new PrometheusMetricsClient("127.0.0.1", pulsar.getListenPortHTTP().get());

Get the Metrics:

Metrics metrics = prometheusMetricsClient.getMetrics();

and assert example:

Metric backlogAgeMetric = metrics.findSingleMetricByNameAndLabels("pulsar_storage_backlog_age_seconds", Pair.of("topic", topic1)); assertThat(backlogAgeMetric.tags).containsExactly( entry("cluster", CLUSTER_NAME), entry("namespace", namespace), entry("topic", topic1)); assertThat((long) backlogAgeMetric.value).isCloseTo(expectedMessageAgeSeconds, within(2L));

Add the method you lack at Metrics

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/BrokerRestEndpointMetricsTest.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

asafm · 2024-03-06T17:20:05Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+    private RestEndpointMetricsFilter(PulsarService pulsar) {
+        PulsarBrokerOpenTelemetry telemetry = pulsar.getOpenTelemetry();
+        Meter meter = telemetry.getMeter();
+        latency = meter.histogramBuilder("pulsar_broker_rest_endpoint_latency")


Please look at #22058 to understand how to do:
Instrument name, description, unit.

Still relevant :)

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

dao-jun · 2024-03-12T18:01:41Z

@dragosvictor Could you please take a look that why my test keeps failing?

asafm · 2024-03-14T11:19:28Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+    private RestEndpointMetricsFilter(PulsarService pulsar) {
+        PulsarBrokerOpenTelemetry telemetry = pulsar.getOpenTelemetry();
+        Meter meter = telemetry.getMeter();
+        latency = meter.histogramBuilder("pulsar_broker_rest_endpoint_latency")


Still relevant :)

asafm · 2024-03-14T11:21:13Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+        Meter meter = openTelemetry.getMeter();
+        latency = meter.histogramBuilder("pulsar_broker_rest_endpoint_latency")
+                .setDescription("Latency of REST endpoints in Pulsar broker")
+                .setUnit("ms")


Should be s, and measured in seconds: See https://opentelemetry.io/docs/specs/semconv/general/metrics/#instrument-units

asafm · 2024-03-14T11:22:27Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+                .build();
+    }
+
+    private static volatile RestEndpointMetricsFilter instance;


Why does it need to be static? Can't you just created one instance of the filter and register it?

asafm · 2024-03-14T11:24:36Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+            UriRoutingContext info = (UriRoutingContext) req.getUriInfo();
+            attrs = getRequestAttributes(info, statusCode);
+        } catch (Throwable ex) {
+            attrs = Attributes.of(PATH, "UNKNOWN", METHOD, req.getMethod(), CODE, (long) statusCode);


Not sure about that yet.

asafm · 2024-03-14T11:43:38Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+
+        Object o = req.getProperty(REQUEST_START_TIME);
+        if (o instanceof Long start) {
+            long duration = System.currentTimeMillis() - start;


I read online now, and from from what I can gather, it's not recommended to use System.currentTimeMillis(). It's a native call to obtain the wall clock time. NTP for example can sync the clock and change. Not 100% sure but leap second may also change it. See: https://develotters.com/posts/how-not-to-measure-elapsed-time/

I think it is safer to use System.nanoTime()

asafm · 2024-03-14T13:27:30Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+            UriRoutingContext info = (UriRoutingContext) req.getUriInfo();
+            attrs = getRequestAttributes(info, statusCode);
+        } catch (Throwable ex) {
+            attrs = Attributes.of(PATH, "UNKNOWN", METHOD, req.getMethod(), CODE, (long) statusCode);


I don't think it's a good practice. For example OOME will vanish like that.
I think if we don't have any specific exception in mind, no point in guarding this part.

asafm · 2024-03-14T13:42:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+        if (CollectionUtils.isEmpty(templates)) {
+            return Attributes.of(PATH, "UNKNOWN", METHOD, httpMethod, CODE, statusCode);
+        }
+        UriTemplate[] arr = templates.toArray(new UriTemplate[0]);


Why do you specifically need to convert this to an array? Can't you just iterate over the list using get(i)?

asafm · 2024-03-14T13:43:37Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java

+        UriTemplate[] arr = templates.toArray(new UriTemplate[0]);
+        int idx = arr.length - 1;
+        StringBuilder builder = new StringBuilder();
+        for (; idx >= 0; idx--) {


Maybe for (int idx = arr.length - 1; ...?

asafm · 2024-03-14T13:49:06Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/BrokerRestEndpointMetricsTest.java

+        admin.tenants().deleteTenant("test");
+
+        Collection<MetricData> metricDatas = pulsarTestContext.getOpenTelemetryMetricReader().collectAllMetrics();
+        log.info("Metrics size: {}", metricDatas.size());


Can be removed at this stage

asafm · 2024-03-14T13:52:50Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/BrokerRestEndpointMetricsTest.java

+
+        Collection<MetricData> metricDatas = pulsarTestContext.getOpenTelemetryMetricReader().collectAllMetrics();
+        log.info("Metrics size: {}", metricDatas.size());
+        Optional<MetricData> optional = metricDatas.stream().peek(m -> log.info("metric name: {}", m.getName()))


OpenTelemetry SDK has a beautiful AssertJ extension for MetricData. @dragosvictor used it in his PR. It's here: https://github.com/open-telemetry/opentelemetry-java/blob/f83c020d4d2a11f16be8af739790718bb3413cba/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/assertj/OpenTelemetryAssertions.java#L50

PIP-223

453762f

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Dec 20, 2023

dao-jun mentioned this pull request Dec 20, 2023

[feat][monitor] PIP-223: Add metrics for all rest endpoints. dao-jun/pulsar#6

Closed

dao-jun added area/broker area/metrics type/PIP labels Dec 20, 2023

dao-jun self-assigned this Dec 20, 2023

dao-jun mentioned this pull request Dec 20, 2023

[feat] PIP-223: Add metrics for all Rest Endpoints. Part1 #18836

Closed

4 tasks

dao-jun changed the title ~~[feat][monitoring] PIP-223: Add metrics for all rest endpoints.~~ [feat][monitor] PIP-223: Add metrics for all rest endpoints. Dec 20, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

dao-jun added the ready-to-test label Jan 22, 2024

dao-jun closed this Feb 29, 2024

dao-jun reopened this Feb 29, 2024

github-actions bot removed the type/PIP label Feb 29, 2024

dao-jun added 2 commits February 29, 2024 16:57

Merge branch 'master' into dev/pip-223

e12f6af

fix code conflicts

e139404

dao-jun requested review from asafm and lhotari February 29, 2024 13:24

asafm requested changes Mar 3, 2024

View reviewed changes

dao-jun added 3 commits March 4, 2024 16:38

Merge branch 'master' into dev/pip-223

978498a

OTEL

395c780

fix var name

552a4ac

asafm reviewed Mar 6, 2024

View reviewed changes

dao-jun added 3 commits March 7, 2024 06:29

Merge branch 'master' into dev/pip-223

5c711c3

fix var name

bc18d11

fix code style

f894da9

asafm reviewed Mar 12, 2024

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestEndpointMetricsFilter.java Show resolved Hide resolved

dao-jun added 4 commits March 12, 2024 19:55

fix test

f8c3f80

fix test

5b9bf65

fix test

434ff8d

fix test

94e8653

asafm reviewed Mar 14, 2024

View reviewed changes

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 11, 2024

lhotari added the release/4.0.1 label Oct 11, 2024

lhotari added release/4.0.2 and removed release/4.0.1 labels Dec 6, 2024

lhotari added release/4.0.3 and removed release/4.0.2 labels Jan 6, 2025

lhotari added release/4.0.4 and removed release/4.0.3 labels Feb 19, 2025

lhotari added release/4.0.5 and removed release/4.0.4 labels Mar 31, 2025

lhotari added release/4.0.6 and removed release/4.0.5 labels May 21, 2025

lhotari added release/4.0.7 and removed release/4.0.6 labels Jul 30, 2025

coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025

lhotari added release/4.0.8 and removed release/4.0.7 labels Sep 27, 2025

lhotari added release/4.0.9 and removed release/4.0.8 labels Nov 11, 2025

Conversation

dao-jun commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

dao-jun commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 29, 2024

Codecov Report

Uh oh!

asafm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dao-jun commented Mar 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dao-jun commented Dec 20, 2023 •

edited

Loading

dao-jun commented Dec 20, 2023 •

edited

Loading

asafm left a comment •

edited

Loading