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.

Sign up for GitHub

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

Jump to bottom

Emit action metrics for batch of markers #4905

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Emit action metrics for batch of markers #4905

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions common/rpc/interceptor/telemetry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,7 @@ func (ti *TelemetryInterceptor) emitActionMetric(
result interface{},
) {
if _, ok := grpcActions[methodName]; !ok || !strings.HasPrefix(fullName, api.WorkflowServicePrefix) {
// grpcActions checks that methodName is the one that we care about.
// ti.scopes verifies that the scope is the one we intended to emit action metrics.
// This is necessary because TelemetryInterceptor is used for all services. Different service could have same
// method name. But we only want to emit action metrics from frontend.
// grpcActions checks that methodName is the one that we care about, and we only care about WorkflowService.
return
}

Expand All @@ -217,11 +214,13 @@ func (ti *TelemetryInterceptor) emitActionMetric(
return
}

hasMarker := false
for _, command := range completedRequest.Commands {
if _, ok := commandActions[command.CommandType]; ok {
switch command.CommandType {
case enums.COMMAND_TYPE_RECORD_MARKER:
// handle RecordMarker command, they are used for localActivity, sideEffect, versioning etc.
hasMarker = true
markerName := command.GetRecordMarkerCommandAttributes().GetMarkerName()
metricsHandler.Counter(metrics.ActionCounter.GetMetricName()).Record(1, metrics.ActionType("command_RecordMarker_"+markerName))
default:
Expand All @@ -230,6 +229,13 @@ func (ti *TelemetryInterceptor) emitActionMetric(
}
}
}
if hasMarker {
// Emit separate action metric for batch of markers.
// One workflow task response may contain multiple marker commands. Each marker will emit one
// command_RecordMarker_Xxx action metric. Depending on pricing model, you may want to ignore all individual
// command_RecordMarker_Xxx and use command_BatchMarkers instead.
metricsHandler.Counter(metrics.ActionCounter.GetMetricName()).Record(1, metrics.ActionType("command_BatchMarkers"))
Copy link
Member

Choose a reason for hiding this comment

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

so action metric will double count? and people need to change the query to exclude either all of command_RecordMarker_Xxx or command_BatchMarkers to get the right count?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Current OSS action metrics include all commands, but not all commands are billable actions.

Copy link
Member

Choose a reason for hiding this comment

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

cc @feihuang after ^ is deployed, the sum of action metric across action_type will double count.

}

case pollActivityTaskQueue:
// handle activity retries
Expand Down
Loading