[feat][monitor] PIP-223: Add metrics for all rest endpoints. #21772

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

asafm · 2024-03-10T10:06:48Z

@dao-jun I'm trying to find a more elegant way to obtain the path template in Jersey DEV mailing list: https://www.eclipse.org/lists/jersey-dev/msg00370.html

asafm · 2024-03-10T13:10:07Z

@dao-jun Ping me when you the PR is ready to continue review.

dao-jun · 2024-03-11T12:43:40Z

@asafm I've updated the PR, PTAL

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 lhotari and asafm 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

dao-jun added 4 commits March 11, 2024 19:49

Update code

aa513b1

Merge branch 'master' into dev/pip-223

7d1bb73

Fix test

76dcde1

Fix code

f414210

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][monitor] PIP-223: Add metrics for all rest endpoints. #21772

[feat][monitor] PIP-223: Add metrics for all rest endpoints. #21772

dao-jun commented Dec 20, 2023 •

edited

Loading

dao-jun commented Dec 20, 2023 •

edited

Loading

codecov-commenter commented Feb 29, 2024

asafm left a comment •

edited

Loading

asafm Mar 3, 2024

asafm Mar 6, 2024

asafm Mar 14, 2024

asafm commented Mar 10, 2024

asafm commented Mar 10, 2024

dao-jun commented Mar 11, 2024

dao-jun commented Mar 12, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

asafm Mar 14, 2024

[feat][monitor] PIP-223: Add metrics for all rest endpoints. #21772

Are you sure you want to change the base?

[feat][monitor] PIP-223: Add metrics for all rest endpoints. #21772

Conversation

dao-jun commented Dec 20, 2023 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

dao-jun commented Dec 20, 2023 • edited Loading

codecov-commenter commented Feb 29, 2024

Codecov Report

asafm left a comment • edited Loading

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

asafm commented Mar 10, 2024

asafm commented Mar 10, 2024

dao-jun commented Mar 11, 2024

dao-jun commented Mar 12, 2024

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 Dec 20, 2023 •

edited

Loading

dao-jun commented Dec 20, 2023 •

edited

Loading

asafm left a comment •

edited

Loading