feat(BREV-1938): add logging options for Shadeform Client#49
feat(BREV-1938): add logging options for Shadeform Client#49patelspratik merged 6 commits intomainfrom
Conversation
| // Filter the list down to the instance types that are allowed by the configuration filter and the args | ||
| for _, singleInstanceType := range instanceTypesFromShadeformInstanceType { | ||
| if !isSelectedByArgs(singleInstanceType, args) { | ||
| c.logger.Debug(ctx, "instance type not selected by args", v1.LogField("instanceType", singleInstanceType.Type)) |
There was a problem hiding this comment.
I think one will be too chatty off the bat, there can be a lot of instance types getting fetched in big batches.
|
|
||
| if keyPairName == "" { | ||
| keyPairName = uuid.New().String() | ||
| c.logger.Debug(ctx, "No key pair name provided, generating new one", v1.LogField("keyPairName", keyPairName)) |
| // convertShadeformInstanceTypeToV1InstanceTypes - converts a shadeform returned instance type to a specific instance type and region of availability | ||
| func (c *ShadeformClient) convertShadeformInstanceTypeToV1InstanceType(shadeformInstanceType openapi.InstanceType) ([]v1.InstanceType, error) { | ||
| func (c *ShadeformClient) convertShadeformInstanceTypeToV1InstanceType(ctx context.Context, shadeformInstanceType openapi.InstanceType) ([]v1.InstanceType, error) { | ||
| c.logger.Debug(ctx, "converting shadeform instance type to v1 instance type", v1.LogField("shadeformInstanceType", shadeformInstanceType)) |
There was a problem hiding this comment.
This also may be too much to start with as it's used in a large loop
| func (c *ShadeformCredential) MakeClientWithOptions(_ context.Context, _ string, opts ...ShadeformClientOption) (v1.CloudClient, error) { | ||
| return NewShadeformClient(c.RefID, c.APIKey, opts...), nil | ||
| } |
There was a problem hiding this comment.
This could instead call:
return c.MakeClient(ctx, location), nil|
|
||
| cloudCredRefID := tags[cloudCredRefIDTagName] | ||
| if err != nil { | ||
| return nil, errors.New("could not find cloudCredRefID tag") | ||
| cloudCredRefID, found := tags[cloudCredRefIDTagName] | ||
| if !found { | ||
| return nil, errors.WrapAndTrace(errors.New("could not find cloudCredRefID tag")) | ||
| } |
| return nil, errors.New("could not find refID tag") | ||
| return nil, errors.WrapAndTrace(errors.New("could not find refID tag")) | ||
| } | ||
| c.logger.Debug(ctx, "refID found, deleting from tags", v1.LogField("refID", refID)) |
There was a problem hiding this comment.
Followup for when this gets consumed by dev-plane:
it would be nice for these logs to be very obviously "shadeform" (and for other providers, very obviously "[provider]"), without needing each implementer to remember to associate their name/ID with each and every log line.
One way we could do that is to adjust the creation of the logger itself (over in dev-plane) to always add span attributes that indicate the provider / cloud cred ID. Maybe instead of:
dev-plane: internal/cloud/cloudsdkv1adapter/adapter_logger.go:
type AdapterLogger struct{}
func NewAdapterLogger() *AdapterLogger {
return &AdapterLogger{}
}we have:
type AdapterLogger struct{
commonFields []zapcore.Field
}
func NewAdapterLogger(commonFields ...zapcore.Field) *AdapterLogger {
return &AdapterLogger{
commonFields: commonFields,
}
}then we could change the fieldsToZapFields to something like:
func (a *AdapterLogger) fieldsToZapFields(fields []cloudv1.Field) []zapcore.Field {
zapFields := []zapcore.Field{}
zapFields = append(zapFields, a.commonFields...)
for _, field := range fields {
zapFields = append(zapFields, zap.Any(field.Key, field.Value))
}
return zapFields
}then when the logger is created (over in internal/cloudcred/service.go) instead of:
logger := cloudsdkadapterv1.NewAdapterLogger()
launchpadClient, err := cred.MakeClientWithOptions(ctx, location, launchpadv1.WithLogger(logger))
if err != nil {
return nil, errors.WrapAndTrace(err)
}we could have:
logger := cloudsdkadapterv1.NewAdapterLogger(
zap.String("cloud_cred_id", string(c.ID)),
zap.String("cloud_provider_id", string(c.ProviderID)),
zap.String("location", location),
)
launchpadClient, err := cred.MakeClientWithOptions(ctx, location, launchpadv1.WithLogger(logger))
if err != nil {
return nil, errors.WrapAndTrace(err)
}There was a problem hiding this comment.
Created linear ticket: https://linear.app/nvidia/issue/BREV-2053/add-provider-prefix-to-cloud-sdk-logs
2 participants
simple fix too add logging Option for ShadeForm Client