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: expose pending messages and processing rate #79

Merged
merged 4 commits into from
Jun 29, 2022

Conversation

whynowy
Copy link
Member

@whynowy whynowy commented Jun 28, 2022

Signed-off-by: Derek Wang whynowy@gmail.com

Processing rate and pending message are exposed in each vertex's prometheus, which will be used later for auto scaling calculation.

Signed-off-by: Derek Wang <whynowy@gmail.com>
@whynowy whynowy marked this pull request as ready for review June 28, 2022 22:39
@whynowy whynowy requested a review from vigith as a code owner June 28, 2022 22:39
Signed-off-by: Derek Wang <whynowy@gmail.com>
package v1alpha1

// VertexInstance is a wrapper of a vertex instance, which contains the vertex spec and the instance information such as hostname and replica index.
type VertexInstance struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

pkg/metrics/metrics.go Show resolved Hide resolved
}
}
pendingMessages.With(labels).Set(float64(total / num))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we emit these metrics to prometheus too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume you meant shall we display these metrics (they are already exposed/emmited) - This is the tricky part.

The pending_messages and processing_rate represent the vertex level metrics, not a pod, but we are exposing them from every individual pod, this will be a problem when displaying in the dashboard, that we should only display the metrics from one pod (or avg).

Copy link
Member Author

Choose a reason for hiding this comment

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

This the reason of the discussion about should we only expose these metrics from 1 pod.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could expose from all pods, the numbers should be more or less the same.. we can let the user configure how they want to display avg/max/min etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krithika3 WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Vigith that we should expose from all the pods and let the user decide when they are displaying these metrics on the type of stat to be used. We just need to call this out specifically in our doc that these numbers are vertex level and should roughly be the same and the aggregation we need to use is just avg/max/min?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

Signed-off-by: Derek Wang <whynowy@gmail.com>
Signed-off-by: Derek Wang <whynowy@gmail.com>
@whynowy whynowy merged commit be78c52 into numaproj:main Jun 29, 2022
@whynowy whynowy deleted the mee branch June 29, 2022 18:34
whynowy added a commit that referenced this pull request Jul 8, 2022
* feat: expose pending messages and processing rate

Signed-off-by: Derek Wang <whynowy@gmail.com>
yhl25 pushed a commit to yhl25/numaflow that referenced this pull request Sep 1, 2022
* feat: expose pending messages and processing rate

Signed-off-by: Derek Wang <whynowy@gmail.com>

Signed-off-by: Yashash H L <yashash_hl@intuit.com>
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.

None yet

3 participants