pkg/services/orgresolver: track pass/fail metrics#2099
Conversation
✅ API Diff Results -
|
| resp, err := o.client.GetOrganizationFromWorkflowOwner(ctx, req) | ||
| if err != nil { | ||
| if o.failCount != nil { | ||
| o.failCount.Add(ctx, 1) //TODO owner attribute? |
There was a problem hiding this comment.
Are owner attributes helpful? Or just noise with extra cardinality?
There was a problem hiding this comment.
good q, they are helpful. we generally pass a labeler around in the engine to have KV attributes we want. The potential problem is that we can't have high cardinality values, for instance WorkflowExecutionID. So to give clients the option, we could define a map on Config that is optional for clients creating an org resolver.
There was a problem hiding this comment.
But do we actually expect the information to be relevant? I wouldn't think problems at this level would be correlated with particular owners. (and even if they were, that would imply the requests are being received by the server, who should be reporting better information than we can as clients?)
| return cfg.New(logger) | ||
| } | ||
|
|
||
| func (cfg *Config) New(logger log.Logger) (*orgResolver, error) { |
There was a problem hiding this comment.
Why the indirection with exporting New over *Config vs just calling into the exported NewOrgResolverWithClient? The old pattern better supported testing
There was a problem hiding this comment.
I added a new field Meter metric.Meter. The XWithY suffix pattern doesn't scale. Why is testing more difficult?
| if cfg.Meter != nil { | ||
| var err error | ||
| //TODO names | ||
| resolver.passCount, err = cfg.Meter.Int64Counter("foo.bar.TODO.pass") |
There was a problem hiding this comment.
i think just the straight name here is fine, even without dot notations to give a path. i.e. "org_resolver_success" and "org_resolver_fail"
| resp, err := o.client.GetOrganizationFromWorkflowOwner(ctx, req) | ||
| if err != nil { | ||
| if o.failCount != nil { | ||
| o.failCount.Add(ctx, 1) //TODO owner attribute? |
There was a problem hiding this comment.
good q, they are helpful. we generally pass a labeler around in the engine to have KV attributes we want. The potential problem is that we can't have high cardinality values, for instance WorkflowExecutionID. So to give clients the option, we could define a map on Config that is optional for clients creating an org resolver.
2 participants
https://smartcontract-it.atlassian.net/browse/CRE-3887
Support passing a
metric.Meterto the org resolver to report pass/fail metrics.