fix: message count read in forwarder

[veds-g]

Fixes #1026

readMessagesCount in forward.go was adding both data messages as well as higher control messages(for low throughput pipelines with multiple partitions) leading to a higher processing rate calculated and shown for the sink vertex

count of messages read now is just based on data messages

[veds-g]

higher initial rate was seen for map vertex initially with multiple partitions

the same pipeline in the issue with higher throughput showed correct processing rates

[vigith]

Should we separately track control messages?

[vigith]

perhaps let's publish total read metrics and also data messages, the difference should be ctrl mesgs.

[KeranYang]

Should we enable multi-partition for the rater e2e test pipeline so that we catch similar issues in future? https://github.com/numaproj/numaflow/blob/main/test/e2e/functional_test.go#L129

[KeranYang]

Perhaps let's publish total read metrics and also data messages, the difference should be ctrl msgs.

I think the ctrl message is the implementation details of Numaflow so we probably shouldn't expose it to users... Now that we exclude the ctrl messages from the read total metric, it's accurate from the user's standpoint that the pipeline read N data messages. An issue with the current approach could be the pipeline gets throttled because of high load and user can't see the real load from the metric because ctrl msgs are excluded but that won't happen because ctrl msgs are very small portion of a high throughput pipeline.

[vigith]

An issue with the current approach could be the pipeline gets throttled because of high load and user can't see the real load from the metric because ctrl msgs are excluded but that won't happen because ctrl msgs are very small portion of a high throughput pipeline.

We should expose metrics even though it is not useful to users. It is quite important for us to debug if we are seeing higher than normal ctrl messages. We expose a lot of metrics that are not valid to users, e.g., WAL count, etc.

[vigith]

perhaps let's publish total read metrics and also data messages, the difference should be ctrl mesgs.

@veds-g why not just expose total read messages rather than just ctrl-messages?

[veds-g]

@veds-g why not just expose total read messages rather than just ctrl-messages?

no particular reason, I thought we wanted control separately. Should I go for the total instead?

[vigith]

no particular reason, I thought we wanted control separately. Should I go for the total instead?

go for the total because it represents everything.

[vigith]

LGTM. @KeranYang please review

[KeranYang]

LGTM. TY for fixing this issue. There is one comment that I want to address but shouldn't be a blocker for merging this PR. I will work on it myself when I have time.