Instrument calls to GraphQL API and Flaps #2233
Conversation
haileys
force-pushed
the
rpc-instrumentation
branch
from
a8c0ed9 to
f31584a
Compare
| @@ -0,0 +1,50 @@ | |||
| package instrument | |||
There was a problem hiding this comment.
Had to introduce this new package under internal to break what would have been a circular package dependency between api and internal/metrics. I think it's a nice result though.
| } | ||
|
|
||
| func RecordCommandContext(ctx context.Context) { | ||
| mu.Lock() | ||
| defer mu.Unlock() |
There was a problem hiding this comment.
unrelated to this PR, but found we were missing mutex lock here. Unlikely to cause real problems, but strictly speaking, it is necessary.
|
The lint check is currently failing with: I don't understand Go modules well enough to know what the problem is here. Anyone have any tips for me? |
api/client.go
Outdated
| @@ -12,6 +12,7 @@ import ( | |||
| "strings" | |||
|
|
|||
| genq "github.com/Khan/genqlient/graphql" | |||
| "github.com/superfly/flyctl/internal/instrument" | |||
There was a problem hiding this comment.
In Go, packages (things you declare with package xxx) and modules (things on go.mod) are different. Module-wise, github.com/superfly/flyctl/api depends github.com/superfly/flyctl but the latter also depends the former.
If flyctl mostly calls RunWithContext() and few methods, you can define a wrapper struct like https://github.com/containerd/containerd/blob/6020903f2c2f0b6b148b02ca7bdd1cd9c6efac3d/pkg/cri/instrument/instrumented_service.go#L50 to call GraphQL.Begin/End. Another option may be defining an interface for instrumentation and pass its implementation, which may be something similar to https://github.com/open-telemetry/opentelemetry-go.
There was a problem hiding this comment.
Oh interesting. It looks like api is a special case here being mentioned in go.mod?
haileys
force-pushed
the
rpc-instrumentation
branch
from
8b2910d to
5e447c4
Compare
No description provided.