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

streamingest: add a replication lag metric #95248

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

lidorcarmel
Copy link
Contributor

@lidorcarmel lidorcarmel commented Jan 14, 2023

Add a metric to track the lag of the replication frontier, in seconds.

Informs: #92959

Epic: CRDB-18752

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lidorcarmel lidorcarmel force-pushed the lidor_metric_lag branch 2 times, most recently from 4f071a8 to 81d5e48 Compare January 17, 2023 23:14
@lidorcarmel lidorcarmel requested a review from a team January 17, 2023 23:14
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @lidorcarmel, and @stevendanna)


pkg/ccl/streamingccl/streamingest/metrics.go line 96 at r1 (raw file):

	}
	metaFrontierLagSeconds = metric.Metadata{
		Name:        "replication.frontier_lag_seconds",

just want to confirm that we'll probably have more replication. prefixed metrics in the future, right? this seems like the first one so want to make sure a new prefix is needed instead of filing it under streaming.replication.* for instance.

@@ -221,6 +221,7 @@ func (h *heartbeatSender) startHeartbeatLoop(ctx context.Context) {
case frontier := <-h.frontierUpdates:
h.frontier.Forward(frontier)
}
metrics.FrontierLagSeconds.Update(timeutil.Since(h.frontier.GoTime()).Seconds())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we think it is better to put this here rather than in maybeUpdatePartitionProgress? If so, some commentary on why might be good.

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 your suggestion would be better, thanks! done. PTAL.

Add a metric to track the lag of the replication frontier, in seconds.

Informs: cockroachdb#92959

Epic: CRDB-18752

Release note: None
@lidorcarmel
Copy link
Contributor Author

pkg/ccl/streamingccl/streamingest/metrics.go line 96 at r1 (raw file):

	}
	metaFrontierLagSeconds = metric.Metadata{
		Name:        "replication.frontier_lag_seconds",

just want to confirm that we'll probably have more replication. prefixed metrics in the future, right? this seems like the first one so want to make sure a new prefix is needed instead of filing it under streaming.replication.* for instance.

right, it was intentional because we want to rename streaming to replication but.. this looks Frankensteiny so probably better to stay consistent with streaming until we do the rename, whenever that is. updated, thanks.

@lidorcarmel
Copy link
Contributor Author

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jan 18, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 18, 2023

Build failed (retrying...):

@craig craig bot merged commit 075b319 into cockroachdb:master Jan 18, 2023
@craig
Copy link
Contributor

craig bot commented Jan 18, 2023

Build succeeded:

@lidorcarmel lidorcarmel deleted the lidor_metric_lag branch January 24, 2023 20:06
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