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.

Sign up for GitHub

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

Jump to bottom

Feature pulsar monitoring #11339

Merged
merged 22 commits into from
Oct 31, 2023
Merged

Feature pulsar monitoring #11339

merged 22 commits into from
Oct 31, 2023

Conversation

liangyepianzhou
Copy link
Contributor

@liangyepianzhou liangyepianzhou commented Sep 17, 2023 •
edited
Loading

image
image
image
image

  • If this is non-trivial feature, paste the links/URLs to the design doc.
  • Update the documentation to include this new feature.
  • Tests(including UT, IT, E2E) are added to verify the new feature.
  • [] If it's UI related, attach the screenshots below.
  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@wu-sheng wu-sheng added backend OAP backend related. feature New feature labels Sep 17, 2023
@wu-sheng
Copy link
Member

Please ref apache/skywalking-booster-ui#318 to submit PR for the translations of the menu words.

@wu-sheng
Copy link
Member

Once this is done, please attach the screenshots. One more optional step is adding the monitoring in the showcase repository.

@wu-sheng
Copy link
Member

CI Licence header check fails,

ERROR the following files don't have a valid license header: 
oap-server/server-starter/src/main/resources/otel-rules/pulsar/pulsar-bookkeeper.yaml
oap-server/server-starter/src/main/resources/otel-rules/pulsar/pulsar-broker.yaml
oap-server/server-starter/src/main/resources/otel-rules/pulsar/pulsar-cluster.yaml
test/e2e-v2/cases/pulsar/pulsar-cases.yaml 
ERROR one or more files does not have a valid license header

And e2e all fails, please make sure you have run the tests locally through e2e tools, and submit the update.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 25, 2023 •
edited
Loading

image

Recommend using MB at least to show the size. I think, from today's infra resource, less than 1MB is not worth showing as an increment.
I am not sure whether these are only disk size relative metrics. If there are more, please consider to align too.

wu-sheng
wu-sheng reviewed Oct 26, 2023
View reviewed changes
docs/menu.yml Outdated Show resolved Hide resolved
@liangyepianzhou
Copy link
Contributor Author

@liangyepianzhou Besides discussion around e2e, are all other comments resolved?

There is still one comment; I am still determining where the unit should be added. Title or other special place.

@wu-sheng
Copy link
Member

@liangyepianzhou Besides discussion around e2e, are all other comments resolved?

There is still one comment; I am still determining where the unit should be added. Title or other special place.

Your link doesn't work. Which one are you referring to?

@wankai123
Copy link
Member

@liangyepianzhou Besides discussion around e2e, are all other comments resolved?

There is still one comment; I am still determining where the unit should be added. Title or other special place.

you can refer to the other dashboard templates, I think add on the title is good.

@liangyepianzhou
Copy link
Contributor Author

@wu-sheng @wankai123 @liuhaoyang All the comments are addressed. Could you please take a look? We are asked to merge this PR tonight.

@wu-sheng
Copy link
Member

We could try, the deadline is 23:59:59 Oct. 31st, UTC+8. So, there are more than 24 hours.

wu-sheng
wu-sheng reviewed Oct 30, 2023
View reviewed changes
docs/menu.yml Outdated Show resolved Hide resolved
@wankai123
Copy link
Member

In the root dashboards, the description text should be on the top, please check and fix both of 2.
image

wankai123
wankai123 reviewed Oct 31, 2023
View reviewed changes
docs/en/setup/backend/backend-bookkeeper-monitoring.md Outdated
| Bookie Read Cache Size | meter_bookkeeper_bookie_read_cache_size | The size of the bookie read cache. | Bookkeeper Cluster |
| Bookie Read Cache Entry Count | meter_bookkeeper_bookie_read_cache_count | The entry count in the bookie read cache. | Bookkeeper Cluster |
| Bookie Read Rate | meter_bookkeeper_bookie_read_rate | The bookie read rate. | Bookkeeper Cluster |
| Bookie Write Rate | meter_bookkeeper_bookie_write_rate | The bookie write rate. | Bookkeeper Cluster |
Copy link
Member

Choose a reason for hiding this comment

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

The unit should be byte/s ? Please indicate and add on the dashboard.

...r/server-starter/src/main/resources/ui-initialized-templates/bookkeeper/bookkeeper-node.json Outdated
],
"widget": {
"name": "thread_executor_completed",
"title": "Thread Executor"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"title": "Thread Executor"
"title": "Thread Executor Completed"

...r/server-starter/src/main/resources/ui-initialized-templates/bookkeeper/bookkeeper-node.json Outdated
},
"widget": {
"name": "jvm_gc_collection_seconds_sum",
"title": "GC Time (ms/min)"
Copy link
Member

Choose a reason for hiding this comment

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

please confirm the unit is ms/min, I see the MAL is the sum of the seconds.

...server/server-starter/src/main/resources/ui-initialized-templates/pulsar/pulsar-cluster.json Outdated
"tips": "Incoming message rate.",
"name": "message_rate_in"
},
"expressions": [],
Copy link
Member

Choose a reason for hiding this comment

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

Please use MQE and check all dashboards config.

...server/server-starter/src/main/resources/ui-initialized-templates/pulsar/pulsar-cluster.json Outdated
},
"widget": {
"name": "storage_logical_size",
"title": "Storage Logical Size"
Copy link
Member

Choose a reason for hiding this comment

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

please don't mark resolved unless you really fix it

docs/en/setup/backend/backend-pulsar-monitoring.md
| Storage Size | meter_pulsar_storage_size | The total storage size of all topics in this broker (in bytes). | Pulsar Cluster |
| Storage Logical Size | meter_pulsar_storage_logical_size | The storage size of all topics in this broker without replicas (in bytes). | Pulsar Cluster |
| Storage Write Rate | meter_pulsar_storage_write_rate | The total message batches (entries) written to the storage for this broker (message batch per second). | Pulsar Cluster |
| Storage Read Rate | meter_pulsar_storage_read_rate | The total message batches (entries) read from the storage for this broker (message batch per second). | Pulsar Cluster |
Copy link
Member

Choose a reason for hiding this comment

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

The units such as (message per second) you add to the description should be added to the dashboards too.

oap-server/server-starter/src/main/resources/ui-initialized-templates/pulsar/pulsar-broker.json
},
"widget": {
"name": "jvm_gc_collection_seconds_sum",
"title": "GC Time (ms/min)"
Copy link
Member

Choose a reason for hiding this comment

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

Check the unit whether it is correct

wankai123
wankai123 reviewed Oct 31, 2023
View reviewed changes
docs/en/setup/backend/backend-bookkeeper-monitoring.md Outdated
| Bookie Read Rate | meter_bookkeeper_bookie_read_rate | The bookie read rate. | Bookkeeper Cluster |
| Bookie Write Rate | meter_bookkeeper_bookie_write_rate | The bookie write rate. | Bookkeeper Cluster |
| Bookie Read Rate | meter_bookkeeper_bookie_read_rate | The bookie read rate (MB/min). | Bookkeeper Cluster |
| Bookie Write Rate | meter_bookkeeper_bookie_write_rate | The bookie write rate (MB/min). | Bookkeeper Cluster |
Copy link
Member

@wankai123 wankai123 Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The MAL rate means: rate(Duration): Calculates the per-second average rate of increase in the time range.
The rate('PT1M') does not mean per-min. It's per-second increase in 1 min

@wu-sheng
Copy link
Member

@liangyepianzhou You can't change things blindly. It would only make you much slower.

wankai123
wankai123 approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@wankai123 wankai123 left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng wu-sheng merged commit ced3f22 into apache:master Oct 31, 2023
167 checks passed
@liangyepianzhou liangyepianzhou deleted the pulsar branch October 31, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng left review comments

@liuhaoyang liuhaoyang liuhaoyang left review comments

@wankai123 wankai123 wankai123 approved these changes

Assignees
No one assigned
Labels
backend OAP backend related. feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liangyepianzhou @wu-sheng @kezhenxu94 @wankai123 @liuhaoyang