Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Sep 21, 2023

This adds metrics for the notices that we have about indexes:

  • Index too wide for a literal constraint.
  • Index with an empty key.
  • Dropping an in-use index.

This will make it easier to prioritize follow-up work, e.g., adding other similar notices, and surfacing these notices properly in built-in tables and the Web Console.

I also sneaked in a notice message change discussed here.

Motivation

  • This PR adds a feature that has not yet been specified.

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

@ggevay ggevay added A-optimization Area: query optimization and transformation A-compute Area: compute labels Sep 21, 2023
@ggevay ggevay requested a review from a team September 21, 2023 12:46
@ggevay ggevay requested a review from a team as a code owner September 21, 2023 12:46
@ggevay ggevay requested a review from jkosh44 September 21, 2023 12:46
session.add_notice(AdapterNotice::DroppedInUseIndex(dropped_in_use_index));
self.metrics
.optimization_notices
.with_label_values(&["DroppedInUseIndex"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if adding a value should be preferred over adding a separate metric per notice type. cc @teskje what would be better for minimizing the number of metrics?

Copy link
Contributor

Choose a reason for hiding this comment

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

the # of time series will be the same either way (the name && labels together are uniquely identifying). what's here looks great

Copy link
Contributor

@umanwizard umanwizard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jkosh44 jkosh44 left a comment

Choose a reason for hiding this comment

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

Adapter code LGTM

Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

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

LGTM!

@ggevay ggevay enabled auto-merge (rebase) September 21, 2023 14:21
@ggevay
Copy link
Contributor Author

ggevay commented Sep 21, 2023

Thanks for the reviews!

@ggevay ggevay merged commit a974df9 into MaterializeInc:main Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-compute Area: compute A-optimization Area: query optimization and transformation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants