Skip to content

[fix][monitor] topic with double quote breaks the prometheus format - #20230

Merged
nicoloboschi merged 1 commit into
apache:masterfrom
nicoloboschi:escape-metricds
May 10, 2023
Merged

[fix][monitor] topic with double quote breaks the prometheus format#20230
nicoloboschi merged 1 commit into
apache:masterfrom
nicoloboschi:escape-metricds

Conversation

@nicoloboschi

@nicoloboschi nicoloboschi commented May 5, 2023

Copy link
Copy Markdown
Contributor

Motivation

If you create a topic with the double quote in the name (e.g. public/default/"mytopic) then there are some metrics that are generated in a broken format.

pulsar_subscriptions_count{cluster="mycluster",namespace="namespace1",topic="persistent://public/default/"mytopic"} 0.0

This leads to errors in the grafana dashboards.

Modifications

  • Escape all label values when generating the metrics

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

@nicoloboschi nicoloboschi self-assigned this May 5, 2023
@nicoloboschi nicoloboschi added this to the 3.1.0 milestone May 5, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 5, 2023
@nicoloboschi nicoloboschi mentioned this pull request May 5, 2023
Closed
This was referenced May 5, 2023
Open
Open
michaeljmarshall
michaeljmarshall approved these changes May 8, 2023
View reviewed changes

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@nicoloboschi

nicoloboschi commented May 8, 2023

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@mattisonchao

mattisonchao commented May 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Weird topic name, it should be illegal after applying PIP #19239.

eolivelli
eolivelli approved these changes May 8, 2023
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

eolivelli
eolivelli approved these changes May 8, 2023
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@michaeljmarshall

michaeljmarshall commented May 9, 2023

Copy link
Copy Markdown
Member

/pulsarbot rerun-failure-checks

@nicoloboschi nicoloboschi reopened this May 10, 2023
@codecov-commenter

codecov-commenter commented May 10, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 38.40%. Comparing base (0dd238a) to head (3aed212).
Report is 1886 commits behind head on master.

Files with missing lines Patch % Lines
...oker/stats/prometheus/PrometheusMetricStreams.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20230      +/-   ##
============================================
+ Coverage     34.48%   38.40%   +3.91%     
- Complexity    12537    13149     +612     
============================================
  Files          1614     1691      +77     
  Lines        126170   131348    +5178     
  Branches      13771    14519     +748     
============================================
+ Hits          43509    50440    +6931     
+ Misses        77053    74488    -2565     
- Partials       5608     6420     +812
Flag Coverage Δ
inttests 24.87% <0.00%> (+0.73%) ⬆️
systests 25.38% <0.00%> (?)
unittests 34.13% <75.00%> (+1.02%) ⬆️

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

Files with missing lines Coverage Δ
...oker/stats/prometheus/PrometheusMetricStreams.java 95.45% <75.00%> (-4.55%) ⬇️

... and 407 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nicoloboschi
nicoloboschi merged commit ea56197 into apache:master May 10, 2023
@nicoloboschi
nicoloboschi deleted the escape-metricds branch May 11, 2023 06:47
nicoloboschi added a commit that referenced this pull request May 11, 2023
@nicoloboschi
[fix][monitor] topic with double quote breaks the prometheus format (#…
f94c8cb 
…20230)

(cherry picked from commit ea56197)
nicoloboschi added a commit that referenced this pull request May 11, 2023
@nicoloboschi
[fix][monitor] topic with double quote breaks the prometheus format (#…
d8bf38e 
…20230)

(cherry picked from commit ea56197)
(cherry picked from commit f94c8cb)
nicoloboschi added a commit that referenced this pull request May 11, 2023
@nicoloboschi
[fix][monitor] topic with double quote breaks the prometheus format (#…
c5b1854 
…20230)

(cherry picked from commit ea56197)
nicoloboschi added a commit that referenced this pull request May 11, 2023
@nicoloboschi
[fix][monitor] topic with double quote breaks the prometheus format (#…
dae05da 
…20230)

(cherry picked from commit ea56197)
(cherry picked from commit c5b1854)
@nicoloboschi nicoloboschi added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label May 11, 2023
nicoloboschi added a commit to datastax/pulsar that referenced this pull request May 11, 2023
@nicoloboschi
[fix][monitor] topic with double quote breaks the prometheus format (a…
95bc8ef 
…pache#20230)

(cherry picked from commit ea56197)
(cherry picked from commit f94c8cb)
(cherry picked from commit d8bf38e)
@nicoloboschi nicoloboschi mentioned this pull request Jun 19, 2023
Merged
5 tasks
@michaeljmarshall

michaeljmarshall commented Jun 27, 2023

Copy link
Copy Markdown
Member

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eolivelli eolivelli eolivelli approved these changes
@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes
@mattisonchao mattisonchao mattisonchao approved these changes
+1 more reviewer
@pgier pgier pgier left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@nicoloboschi nicoloboschi

Labels

cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.5 release/2.11.2 release/3.0.1

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@nicoloboschi @mattisonchao @michaeljmarshall @codecov-commenter @pgier @eolivelli