lostluck commented on a change in pull request #15887:
URL: https://github.com/apache/beam/pull/15887#discussion_r742418826
##########
File path: sdks/go/pkg/beam/core/metrics/metrics.go
##########
@@ -843,13 +872,16 @@ func ResultsExtractor(ctx context.Context) Results {
committed[key] = GaugeValue{opt.(*gauge).v,
opt.(*gauge).t}
r.gauges = append(r.gauges, MergeGauges(attempted,
committed)...)
case *executionState:
- for _, v := range opt.(*executionState).state {
- attempted := make(map[StepKey]MsecValue)
- committed := make(map[StepKey]MsecValue)
- attempted[key] = MsecValue{}
- committed[key] = MsecValue{int64(v.TotalTime)}
- r.msecs = append(r.msecs, MergeMsecs(attempted,
committed)...)
+ attempted := make(map[StepKey]MsecValue)
+ committed := make(map[StepKey]MsecValue)
+ attempted[key] = MsecValue{}
+ committed[key] = MsecValue{[4]time.Duration{0, 0, 0, 0}}
Review comment:
Nit: New allocations for Go values are always their default 0 value. So
`MsecValue{}` and `MsecValue{[4]time.Duration{0, 0, 0, 0}}` are identical.
##########
File path: sdks/go/pkg/beam/core/metrics/metrics.go
##########
@@ -481,7 +481,7 @@ func (m *executionState) kind() kind {
// MsecValue is the value of a single msec metric
type MsecValue struct {
- Time int64
+ Time [4]time.Duration
Review comment:
Consider the following:
We change the type name to DoFnTimes.
Then instead of an array that doesn't indicate any semantics to a user
(which has the total? 0? 3?) we expand those to fields.
```
type DoFnTimes struct{
Start, Process, Finish, Total time.Duration
}
```
This makes it easier for users to understand what they're getting.
##########
File path: sdks/go/pkg/beam/core/metrics/metrics.go
##########
@@ -843,13 +872,16 @@ func ResultsExtractor(ctx context.Context) Results {
committed[key] = GaugeValue{opt.(*gauge).v,
opt.(*gauge).t}
r.gauges = append(r.gauges, MergeGauges(attempted,
committed)...)
case *executionState:
- for _, v := range opt.(*executionState).state {
- attempted := make(map[StepKey]MsecValue)
- committed := make(map[StepKey]MsecValue)
- attempted[key] = MsecValue{}
- committed[key] = MsecValue{int64(v.TotalTime)}
- r.msecs = append(r.msecs, MergeMsecs(attempted,
committed)...)
+ attempted := make(map[StepKey]MsecValue)
+ committed := make(map[StepKey]MsecValue)
+ attempted[key] = MsecValue{}
+ committed[key] = MsecValue{[4]time.Duration{0, 0, 0, 0}}
Review comment:
Nit: New allocations for Go values are always their default 0 value. So
`MsecValue{}` and `MsecValue{[4]time.Duration{0, 0, 0, 0}}` are identical.
##########
File path: sdks/go/pkg/beam/core/metrics/metrics.go
##########
@@ -481,7 +481,7 @@ func (m *executionState) kind() kind {
// MsecValue is the value of a single msec metric
type MsecValue struct {
- Time int64
+ Time [4]time.Duration
Review comment:
Consider the following:
We change the type name to DoFnTimes.
Then instead of an array that doesn't indicate any semantics to a user
(which has the total? 0? 3?) we expand those to fields.
```
type DoFnTimes struct{
Start, Process, Finish, Total time.Duration
}
```
This makes it easier for users to understand what they're getting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
