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

feat(metrics): add counter for retry create stream reader #20258

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tabVersion
Copy link
Contributor

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

follow up #19805

manually test

  1. create table with kafka
  2. stop kafka container
  3. trigger recovery in psql
  4. observe grafana
image

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

Comment on lines +93 to +99
let init_stream_reader_retry_count = register_guarded_int_counter_vec_with_registry!(
"source_init_stream_reader_retry_count",
"Total number of retries to initialize stream reader",
&["actor_id", "source_name",],
registry
)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

should we add this to grafana dashboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it overlaps with source_status_is_up (from source enumerator), so I don't add it to grafana.
But here we focus more on the retry on the compute node, planning add it to cloud alert.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I don't know we have source_status_is_up!

Then what about adding it to the same panel (Source Upstream Status), so that we can both know whether the source is up on meta and each CN. 🤔

Comment on lines +550 to +554
let source_ctx_inner = source_ctx_clone.clone();
source_ctx_inner.metrics.init_stream_reader_retry_count.with_guarded_label_values(&[
&source_ctx_inner.actor_id.to_string(),
&source_ctx_inner.source_name,
]).inc();
Copy link
Member

Choose a reason for hiding this comment

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

Could we simply move this into source_reader.build_stream? And perhaps rename to stream_reader_build_count.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I feel the figure is a little hard to understand. Why did it go up and down 🤔

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am struggling with it too, suspect somewhere calls a rate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the reason is RW is reporting the metric from multiple nodes and the count value from each node varies, leading to the fluctuation.
But it is irrelevant to the alert part, we always alert when the value is non-zero.

Copy link

gru-agent bot commented Jan 23, 2025

This pull request has been modified. If you want me to regenerate unit test for any of the files related, please find the file in "Files Changed" tab and add a comment @gru-agent. (The github "Comment on this file" feature is in the upper right corner of each file in "Files Changed" tab.)

@tabVersion
Copy link
Contributor Author

let's wait for #20295, to minimize the changes.

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.

4 participants