-
Notifications
You must be signed in to change notification settings - Fork 800
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
Refactor metrics with domain tag #2386
Conversation
func (t *timerQueueActiveProcessorImpl) getMetricScopeWithDomainTag(scope int, domainID string) (metrics.Scope, error) { | ||
domainEntry, err := t.shard.GetDomainCache().GetDomainByID(domainID) | ||
if err != nil { | ||
return nil, &workflow.InternalServiceError{Message: "unable to get domain from cache by domainID."} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we should return this error instead of making a new one. Because the err from domainCache could should have more useful info than this simple text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense. updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But please address a small comment before landing it.
* Add new es record for uninitialized workflow (#2386) * Add nil check for visibility manager when adding uninitialized wf execution record (#2386) * Update TestProcessRecordWorkflowStartedTask to include unit test for record wf execution uninitialized method * Fix the test add method of es processor (#2386) * Disable the feature by default (#2386) * Fix the unit test when feature is disabled * Fix the unit test when feature is disabled (#2386)
* Add new es record for uninitialized workflow (uber#2386) * Add nil check for visibility manager when adding uninitialized wf execution record (uber#2386) * Update TestProcessRecordWorkflowStartedTask to include unit test for record wf execution uninitialized method * Fix the test add method of es processor (uber#2386) * Disable the feature by default (uber#2386) * Fix the unit test when feature is disabled * Fix the unit test when feature is disabled (uber#2386)
Small efforts to reduce some duplicates