feat: undefined resolver props and model props to trigger warning logs#8914
feat: undefined resolver props and model props to trigger warning logs#8914
Conversation
begelundmuller
left a comment
begelundmuller
left a comment
There was a problem hiding this comment.
As a reflection on my comments below, I would probably expect a more structured and less log-centric approach to this issue. Some ideas on my mind:
- For parser warnings, change
parser.Parserto gather warnings as state on theParserobject - similar to the currentParser.Errorsfield. Maybe even changeErrorstoIssuesand have alevelorbool warningon theruntimev1.ParseErrortype? This would keep theparserpackage as a stateless library that returns issues instead of producing secondary output. - For resolver args, it would be nicer to validate them during reconciliation of the alert/report/API/model resources; for now, it could log validation errors as warnings, but when Naman's PR lands we could change it to return the warnings as part of the reconcile result so they show up in the status page.
- Lastly, what about supporting new settings like
rill.strict_resolver_propertiesandrill.strict_model_properties, which iftrue, will cause it to error instead of warn for broken properties? We could maybe set this tofalsefor all existing projects, and then make it default totrueso new projects have strict checks.
| // make a minimal struct to only parse partitions properties for validation | ||
| // it should not contain any extra fields | ||
| minimal := struct { | ||
| Partitions *DataYAML `yaml:"partitions"` | ||
| }{} | ||
| // extract partitions field from the original YAML to validate it | ||
| tmpMap := make(map[string]any) |
There was a problem hiding this comment.
Why can't this be done as part of parseDataYAML?
|
|
||
| // Parse creates a new parser and parses the entire project. | ||
| func Parse(ctx context.Context, repo drivers.RepoStore, instanceID, environment, defaultOLAPConnector string) (*Parser, error) { | ||
| func Parse(ctx context.Context, repo drivers.RepoStore, instanceID, environment, defaultOLAPConnector string, logger *zap.Logger) (*Parser, error) { |
There was a problem hiding this comment.
The parser is a stateless library, so logging inside of it feels like an anti-pattern. Logging should ideally only happen from long-running or stateful code (like the server, reconciler, etc.).
| } | ||
| if len(unused) > 0 { | ||
| e.c.logger.Warn("Undefined fields in input properties. Will be ignored", zap.String("model", opts.ModelName), zap.Strings("fields", unused), observability.ZapCtx(ctx)) | ||
| } |
There was a problem hiding this comment.
Logging in the executor is okay, but I'm not sure it's sufficient. It would be nice to be able to bubble up redundant properties as reconcile errors or warnings (when Naman's PR for that lands).
For example, logs are not visible to AI and won't be easy to view in cloud editing, so they are easily missed.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| logUnusedProperties(ctx, opts, "ai", unused) |
There was a problem hiding this comment.
I think logging warnings in each resolver call is excessive. Resolvers are also used in API calls, so it can generate tons of warning logs (which will show up in our internal dashboards, but which are easily missed by the user since they don't usually see server logs).
2 participants
closes https://linear.app/rilldata/issue/PLAT-367/return-errors-for-unsupported-properties-in-resolvers-and-model
Checklist: