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

Add prometheus metric for per partition high watermark #11148

Merged
merged 11 commits into from
Mar 16, 2022

Conversation

nharring-adjacent
Copy link
Contributor

As mentioned in https://github.com/MaterializeInc/cloud/issues/2355 there is a desire to have the broker reported high watermark for each partition available in metrics which was lost when the interaction with kafka was reworked. This PR adds the metric mz_kafka_partition_offset_max with tags for topic, source_id, source_instance and partition_id to the set exported by prometheus which also puts it into the mz_metrics system table.

Motivation

  • This PR adds a known-desirable feature.

https://github.com/MaterializeInc/cloud/issues/2355

Tips for reviewer

No, high watermark is not the correct spelling but its what kafka and rdkafka use so we will all have to wonder exactly what happens to a watermark to get it high or low.

Testing

  • This PR has adequate test coverage / QA involvement has been duly considered.

Release notes

This PR includes the following user-facing behavior changes:

  • Add metric mz_kafka_partition_offset_max which records the per partition high_watermark reported by the broker. This metric is available via the prometheus metrics endpoint and the mz_metrics system table.

@nharring-adjacent nharring-adjacent marked this pull request as ready for review March 9, 2022 23:21
@nharring-adjacent nharring-adjacent requested a review from a team March 9, 2022 23:21
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka/metrics.rs Show resolved Hide resolved
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

The code LGTM, but please add a testdrive test that exercises metric reporting int he face of dynamic partition additions!

src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka/metrics.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka/metrics.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka/metrics.rs Outdated Show resolved Hide resolved
src/dataflow/src/source/kafka.rs Outdated Show resolved Hide resolved
test/metrics/kafka-partition-metrics.td Outdated Show resolved Hide resolved
------------------
0 2
1 2
2 1
Copy link
Member

Choose a reason for hiding this comment

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

This test looks great. Glad it already paid some dividends, too!

test/metrics/mzcompose.py Show resolved Hide resolved
Nick Harring added 2 commits March 16, 2022 10:04
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

🎉

@nharring-adjacent nharring-adjacent merged commit f934dcb into MaterializeInc:main Mar 16, 2022
wangandi pushed a commit to wangandi/materialize that referenced this pull request Apr 11, 2022
…c#11148)

* initial addition of broker high watermark per partition

* lint

* fix rebase

* clean up deserialization

* use cursor instead of raw vec

* Lint

* address feedback

* add tests, fix issues found by tests

* lint

* feedback

* lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants