Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][broker] Reuse topic OpenTelemetry attributes #22876

Merged

Conversation

dragosvictor
Copy link
Contributor

@dragosvictor dragosvictor commented Jun 7, 2024

Fixes #22817

Motivation

Successive collections of OpenTelemetry metrics re-allocate all attribute sets related to a topic. Since topics are a high-cardinality object in Pulsar, this puts pressure on the GC. This PR proposes caching these attribute sets.

Modifications

  • Adds classes TopicAttributes and PersistentTopicAttributes to store all attribute sets relevant for a topic.
  • Add and lazily initialize these attribute fields in the NonPersistentTopic and PersistentTopic objects. If OpenTelemetry is not enabled, these objects never get created, saving memory.
  • Modify OpenTelemetryTopicStats to use the cached objects instead of re-allocating them every run.
  • Relocate some of the attributes into interface OpenTelemetryAttributes, for a cleaner design.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as org.apache.pulsar.broker.stats.OpenTelemetryTopicStatsTest.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: dragosvictor#31

@dragosvictor dragosvictor marked this pull request as ready for review June 7, 2024 16:56
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 7, 2024
@dao-jun dao-jun added this to the 3.4.0 milestone Jun 7, 2024
@dao-jun dao-jun closed this Jun 7, 2024
@dao-jun dao-jun reopened this Jun 7, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work @dragosvictor

@lhotari lhotari requested review from asafm and heesung-sn June 7, 2024 18:31
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 94.25287% with 5 lines in your changes missing coverage. Please review.

Project coverage is 73.25%. Comparing base (bbc6224) to head (e30dae9).
Report is 734 commits behind head on master.

Files with missing lines Patch % Lines
...oker/service/nonpersistent/NonPersistentTopic.java 33.33% 4 Missing ⚠️
...sar/broker/service/persistent/PersistentTopic.java 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22876      +/-   ##
============================================
- Coverage     73.57%   73.25%   -0.33%     
- Complexity    32624    32994     +370     
============================================
  Files          1877     1891      +14     
  Lines        139502   141935    +2433     
  Branches      15299    15565     +266     
============================================
+ Hits         102638   103970    +1332     
- Misses        28908    29939    +1031     
- Partials       7956     8026      +70     
Flag Coverage Δ
inttests 27.25% <87.35%> (+2.67%) ⬆️
systests 24.66% <2.29%> (+0.34%) ⬆️
unittests 72.27% <94.25%> (-0.57%) ⬇️

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

Files with missing lines Coverage Δ
...lsar/broker/service/PersistentTopicAttributes.java 100.00% <100.00%> (ø)
...n/java/org/apache/pulsar/broker/service/Topic.java 33.33% <ø> (-3.04%) ⬇️
.../apache/pulsar/broker/service/TopicAttributes.java 100.00% <100.00%> (ø)
...e/pulsar/broker/stats/OpenTelemetryTopicStats.java 99.06% <100.00%> (ø)
.../pulsar/opentelemetry/OpenTelemetryAttributes.java 100.00% <100.00%> (ø)
...sar/broker/service/persistent/PersistentTopic.java 79.21% <83.33%> (+0.75%) ⬆️
...oker/service/nonpersistent/NonPersistentTopic.java 70.52% <33.33%> (+1.06%) ⬆️

... and 382 files with indirect coverage changes

---- 🚨 Try these New Features:

@heesung-sn heesung-sn merged commit 5a8db36 into apache:master Jun 7, 2024
72 of 75 checks passed
lhotari pushed a commit that referenced this pull request Jun 7, 2024
@dragosvictor dragosvictor deleted the dmisca-pip-264-reuse-topic-attributes branch June 7, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excessive memory allocation in OTel broker metrics
5 participants