Add tags to metrics #414

vancexu · 2018-03-06T18:56:09Z

No description provided.

yiminc-zz

is tasklist something we want to add as well?

yiminc-zz · 2018-03-07T00:21:42Z

internal/internal_logging_tags.go

@@ -36,3 +36,7 @@ const (
 	tagSideEffectID    = "SideEffectID"
 	tagChildWorkflowID = "ChildWorkflowID"
 )
+
+func newTag(tagName, tagValue string) map[string]string {


use tagScope() helper method.

yiminc-zz · 2018-03-07T00:23:29Z

internal/internal_worker.go

@@ -1020,6 +1020,7 @@ func newAggregatedWorker(

 	ensureRequiredParams(&workerParams)
 	workerParams.MetricsScope = tagScope(workerParams.MetricsScope, tagDomain, domain)
+	workerParams.MetricsScope = tagScope(workerParams.MetricsScope, tagTaskList, taskList)


could you update tagScope to take variable inputs so we only call tagScope once and only create one map.

yiminc-zz · 2018-03-07T21:11:51Z

internal/internal_workflow_client.go

@@ -190,7 +190,8 @@ func (wc *workflowClient) StartWorkflow(
 	}

 	if wc.metricsScope != nil {
-		wc.metricsScope.Counter(metrics.WorkflowStartCounter).Inc(1)
+		tagScope(wc.metricsScope, tagWorkflowType, workflowType.Name).


this will only include that tag for this specific metric. We want to include those tags for all metrics. But if you do the same thing for every metric, you are going to create a log of tagged subscope, which is not a cheap operation.

yiminc-zz · 2018-03-14T22:19:24Z

internal/internal_workflow_client.go

+		workflowService   workflowserviceclient.Interface
+		domain            string
+		metricsScope      tally.Scope
+		tagToMetricsScope map[string]tally.Scope // to avoid repeated creation of subScope


this need to handle multi-thread concurrent access issue.

yiminc-zz · 2018-03-14T22:19:58Z

internal/internal_task_handlers.go

@@ -1247,7 +1249,11 @@ func (ath *activityTaskHandlerImpl) Execute(taskList string, t *s.PollForActivit
 			ath.logger.Error("Activity panic.",
 				zap.String("PanicError", fmt.Sprintf("%v", p)),
 				zap.String("PanicStack", st))
-			ath.metricsScope.Counter(metrics.ActivityTaskPanicCounter).Inc(1)
+			activityTypeName := t.ActivityType.GetName()
+			if _, ok := ath.tagToMetricsScope[activityTypeName]; !ok {


extract these code so we don't have to dup this logic in 3 places.

yiminc-zz · 2018-03-14T22:21:28Z

internal/internal_task_handlers.go

+		identity          string
+		service           workflowserviceclient.Interface
+		metricsScope      tally.Scope
+		tagToMetricsScope map[string]tally.Scope // to avoid repeated creation of subScope


type scopeFactory struct {
tagToScopes map[string]tally.Scope
}

func (f *scopeFactory) getScope(tag string) Scope {
//...
}

vancexu · 2018-03-15T21:52:48Z

@yiminc all comments addressed. let me know if you have more concerns thanks.

coveralls · 2018-03-15T21:54:05Z

Coverage decreased (-0.05%) to 65.145% when pulling d39a0d0 on metric into 8e121e2 on master.

yiminc-zz

I would create a new type that encapsulate this logic
type taggedScope struct {
Tally.scope
syncMap sync.Map
}

func (ts *taggedScope) getTaggedScope(tagName, tagvalue) scope {

}

yiminc-zz · 2018-03-15T22:42:27Z

internal/internal_worker.go

+// otherwise retrieved subScope will be corrupted. e.g you cannot getOrCreateTaggedScope(m, scope, "domain", "A name")
+// and then getOrCreateTaggedScope(m, scope, "workflow-type", "A name")
+func getOrCreateTaggedScope(m sync.Map, scope tally.Scope, tagName, tagValue string) tally.Scope {
+	taggedScope, ok := m.Load(tagValue)


can you make the key be tagName+tagValue, then you don't have to worry about the case you put in your comment.

yiminc-zz · 2018-03-16T21:41:26Z

internal/client.go

@@ -326,7 +327,7 @@ func NewClient(service workflowserviceclient.Interface, domain string, options *
 	return &workflowClient{
 		workflowService: metrics.NewWorkflowServiceWrapper(service, metricScope),
 		domain:          domain,
-		metricsScope:    metricScope,
+		metricsScope:    &metrics.TaggedScope{Scope: metricScope, Map: &sync.Map{}},


create a func to create this because who using this taggedScope should not need to know that it needs a sync.Map internally:
func newTaggedScope(scope tally.Scope) *TaggedScope {
return &metrics.TaggedScope{Scope: scope, Map: &sync.Map{}}
}

yiminc-zz · 2018-03-16T21:42:44Z

internal/common/metrics/scope.go

+		ts.Map = &sync.Map{}
+	}
+
+	key := tagName + tagValue // used to prevent collision of tagValue (map key) for different tagName


add a separater here: key := tagName + ":" + tagValue.

yiminc-zz · 2018-03-16T21:45:02Z

internal/common/metrics/scope.go

+}
+
+// NewTaggedScope create a new TaggedScope
+func NewTaggedScope() *TaggedScope {


let this function take a scope as input parameter

yiminc-zz · 2018-03-16T21:45:48Z

internal/internal_task_handlers.go

@@ -1089,7 +1089,7 @@ func newActivityTaskHandlerWithCustomProvider(
 		identity:         params.Identity,
 		service:          service,
 		logger:           params.Logger,
-		metricsScope:     params.MetricsScope,
+		metricsScope:     &metrics.TaggedScope{Scope: params.MetricsScope, Map: &sync.Map{}},


use that newTaggedScope(scope) method

yiminc-zz · 2018-03-16T21:46:29Z

internal/internal_workflow_testsuite.go

@@ -212,7 +213,7 @@ func newTestWorkflowEnvironmentImpl(s *WorkflowTestSuite) *testWorkflowEnvironme
 		env.logger = logger
 	}
 	if env.metricsScope == nil {
-		env.metricsScope = tally.NoopScope
+		env.metricsScope = &metrics.TaggedScope{Scope: s.scope, Map: &sync.Map{}}


use that newTaggedScope(scope) method

yiminc-zz · 2018-03-16T21:46:51Z

internal/internal_workflow_client_test.go

@@ -190,7 +192,7 @@ func (s *workflowRunSuite) SetupTest() {
 	mockCtrl := gomock.NewController(s.T())
 	s.workflowServiceClient = workflowservicetest.NewMockClient(mockCtrl)

-	metricsScope := tally.NoopScope
+	metricsScope := &metrics.TaggedScope{Scope: tally.NoopScope, Map: &sync.Map{}}


use the newTaggedScope(scope) function

yiminc-zz

lgtm

vancexu requested a review from yiminc-zz March 6, 2018 18:56

vancexu force-pushed the metric branch 2 times, most recently from bbaf0dc to d5ee823 Compare March 6, 2018 19:27

yiminc-zz reviewed Mar 7, 2018

View reviewed changes

yiminc-zz suggested changes Mar 14, 2018

View reviewed changes

yiminc-zz reviewed Mar 15, 2018

View reviewed changes

vancexu added 7 commits March 16, 2018 12:27

Add tags to metrics

60f19bd

fix typo

afd0d69

refactor to use tagScope

8ce9280

Avoid repeated creation of subscope

ea90ca4

Use thread safe map

150c96b

fix lint

6100d98

Create new TaggedScope

ed72e73

vancexu force-pushed the metric branch from bd0e70d to ed72e73 Compare March 16, 2018 19:28

yiminc-zz reviewed Mar 16, 2018

View reviewed changes

change NewTaggedScope()

d39a0d0

yiminc-zz approved these changes Mar 16, 2018

View reviewed changes

vancexu merged commit a31deb5 into master Mar 16, 2018

vancexu deleted the metric branch March 16, 2018 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tags to metrics #414

Add tags to metrics #414

vancexu commented Mar 6, 2018

yiminc-zz left a comment

yiminc-zz Mar 7, 2018

vancexu Mar 7, 2018

yiminc-zz Mar 7, 2018

vancexu Mar 7, 2018

yiminc-zz Mar 7, 2018

yiminc-zz Mar 14, 2018

yiminc-zz Mar 14, 2018

yiminc-zz Mar 14, 2018

vancexu commented Mar 15, 2018

coveralls commented Mar 15, 2018 •

edited

Loading

yiminc-zz left a comment

yiminc-zz Mar 15, 2018

yiminc-zz Mar 16, 2018

yiminc-zz Mar 16, 2018

yiminc-zz Mar 16, 2018

yiminc-zz Mar 16, 2018

yiminc-zz Mar 16, 2018

yiminc-zz Mar 16, 2018

yiminc-zz left a comment

Add tags to metrics #414

Add tags to metrics #414

Conversation

vancexu commented Mar 6, 2018

yiminc-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu commented Mar 15, 2018

coveralls commented Mar 15, 2018 • edited Loading

yiminc-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiminc-zz left a comment

Choose a reason for hiding this comment

coveralls commented Mar 15, 2018 •

edited

Loading