feat: metric integration for fsm #2155
Conversation
| package observability | ||
|
|
||
| const ( | ||
| fsmRefAttribute string = "ftl.fsm.ref" |
There was a problem hiding this comment.
Given this is only used by fsm.go it could go there instead of this shared attributes file (unless there's plans to share it in the future).
There was a problem hiding this comment.
If we don't currently have any shared attributes then we can leave this file off for now and add it back if needed in the future.
| func Init() { | ||
| InitFSMMetrics() | ||
| } |
There was a problem hiding this comment.
Maybe this should be something like:
func InitControllerObservability(ctx context.Context) {
err := internalobs.Init(ctx, .....)
// check errors
err := InitFSMMetrics(...)
// check errors
}There was a problem hiding this comment.
adopted that name
internal/observability/client.go
Outdated
| observability.Init() | ||
|
|
There was a problem hiding this comment.
The internal package should not depend on backend/controller/... things at all (or runner, etc.). This would create internal dependencies on controller and runner. Instead the backend stuff should use internal as needed.
There was a problem hiding this comment.
moved to controller.go:New
| @@ -1,18 +1,5 @@ | |||
| package metrics | |||
There was a problem hiding this comment.
I think this file should move up one level and be at the internal/observability level instead of internal/observability/metrics that way we can use shared attributes in traces as well.
| func InitFSMMetrics() { | ||
| fsmCounters.instancesActive, _ = fsmMeter.Int64UpDownCounter( |
There was a problem hiding this comment.
func InitFSMMetrics() should return an error so it can be checked.
There was a problem hiding this comment.
agreed; just going for a quicker feedback turn around on other aspects of the PR by leaving that as a TODO comment, will return to it before exiting draft mode on this PR
There was a problem hiding this comment.
added the error handling
jonathanj-square
force-pushed
the
jonathanj/otel/fsm
branch
from
c185ea9 to
9a4ef38
Compare
| if err := observability.InitControllerObservability(); err != nil { | ||
| log.FromContext(ctx).Warnf("failed to initialize controller observability: %v", err) | ||
| } |
There was a problem hiding this comment.
This works for now, but we might want to consider combining this and the observability.Init() that's currently in ftl-controller/main ftl-runner/main and cmd_serve files now. We can iterate on that in the future.
There was a problem hiding this comment.
will combine in a fast follow change that completes the fsm metric instrumentation
jonathanj-square
force-pushed
the
jonathanj/otel/fsm
branch
from
9a4ef38 to
adea7a1
Compare
|
LGTM I do like @wesbillman simplification to have observability's init also initialize the various metrics. Structurally, could still keep it clean and do something like |
jonathanj-square
requested review from
a team and
matt2e
and removed request for
a team
|
Linking issue: #2194 |
No description provided.