-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(otelhttpclient): move generic otelclient from meteor to salt #16
Conversation
go.mod
Outdated
go 1.16 | ||
go 1.21 | ||
|
||
toolchain go1.21.8 |
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.
do we need this?
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.
removed
telemetry/otelgrpc/otelgrpc.go
Outdated
attributes []attribute.KeyValue | ||
} | ||
|
||
func NewOtelGRPCMonitor(hostName string) Monitor { |
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.
func NewOtelGRPCMonitor(hostName string) Monitor { | |
func NewMeter(hostName string, MeterOpts...) Monitor { |
We should define a MeterOpts to make it optional to pass meter name so creating a new meter would be
otelgrpc.NewMeter("host", otelgrpc.WithMeterName("github.com/goto/compass"))
telemetry/otelgrpc/otelgrpc.go
Outdated
Err error | ||
} | ||
|
||
type Monitor struct { |
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.
type Monitor struct { | |
type Meter struct { |
telemetry/otelgrpc/otelgrpc.go
Outdated
size := proto.Size(p.(proto.Message)) | ||
return size |
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.
should we make it safer?
size := proto.Size(p.(proto.Message)) | |
return size | |
if pm, ok := p.(proto.Message); ok { | |
return proto.Size(pm) | |
} | |
return 0 |
telemetry/otelgrpc/otelgrpc.go
Outdated
return size | ||
} | ||
|
||
func (m *Monitor) RecordUnary(ctx context.Context, p UnaryParams) { |
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.
if we don't use this outside, make it private
func (m *Monitor) RecordUnary(ctx context.Context, p UnaryParams) { | |
func (m *Monitor) recordUnary(ctx context.Context, p UnaryParams) { |
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.
existing unit test call
telemetry/otelgrpc/otelgrpc_test.go
Outdated
"testing" | ||
"time" | ||
|
||
pb "github.com/goto/optimus/protos/gotocompany/optimus/core/v1beta1" |
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.
should use sample proto/server for this
commonv1 "github.com/goto/salt/server/example/proto/gotocompany/common/v1"
telemetry/otelgrpc/otelgrpc.go
Outdated
} | ||
|
||
func NewMeter(hostName string, meterOpts ...MeterOpts) Meter { | ||
meter := otel.Meter("github.com/goto/salt/telemetry/otelgrpc") |
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.
The name should not be fixed like this. The one that uses library should be able to configure this
Migrating generic otelhttpclient library from meteor to salt, for making it available for other repo to use it and avoid duplication