Add relation info to node_info in logging
#7304
Conversation
core/dbt/events/types.proto
Outdated
| string node_database = 10; | ||
| string node_schema = 11; | ||
| string node_alias = 12; | ||
| string node_relation_name = 13; |
There was a problem hiding this comment.
@gshank @peterallenwebb Does either of you have a strong feeling around whether:
- We should prefix these with
node_(like the other ones) - We should nest these as fields in a
relation_infodictionary, instead of having them all as top-level fields (strings)
There was a problem hiding this comment.
tbh I kind of wish none of the other fields started with "node_". It feels duplicative. But that's moot, I guess :)
Nested or not I don't have strong feelings.
I wonder if we need to make these "optional" though. Did you test this with a node with database = None? The version of betterproto we were using before didn't have "optional", and google protobuf behaves a bit differently anyway. I'm not quite sure what it will do with a None passed in. Changing the event in test_events.py should tell.
There was a problem hiding this comment.
How about node_relation: { database='...', schema='', alias='', name: '...' }?
There was a problem hiding this comment.
If I pass in None for database, it comes back as "". I'll add a test for this. I think this is how proto works, right? All fields are technically optional, and there's no way to really distinguish between empty string ("") and missing (None), short of adding another boolean field has_database
There was a problem hiding this comment.
There is an "optional" thing, but it's not exactly the same as in Python, so I suspect we'd still get an empty string. betterproto was trying to be more python-like, so I think they implemented it in a somewhat more pythonic way, but I haven't done any optional fields with google protobuf yet...
There was a problem hiding this comment.
I went with
node_relation: {database='...', schema='...', alias='...', relation_name: '...'}
To match the nomenclature in the existing top-level node attributes, and because name by itself feels closer to alias/identifier
tests/unit/test_proto_events.py
Outdated
| "node_database": "some_database", | ||
| "node_schema": "some_schema", | ||
| "node_alias": "some_alias", | ||
| "node_relation_name": "some.relation.name", |
There was a problem hiding this comment.
I updated this test. Not sure if we would want to update these functional tests as well, to ensure the field values are actually present:
* Add db, schema, alias, rel_name to node_info * Add changelog entry * Default to None * Nested node_relation message
resolves #6724
Description
Add to
node_infodictionary another nested message/dictionary fornode_relation, including:databaseschemaaliasrelation_name(with include + quote policies applied){ "data": { "description": "sql view model dbt_jcohen.my_model", "index": 1, "node_info": { "materialized": "view", "meta": {}, "node_finished_at": "", "node_name": "my_model", "node_path": "my_model.sql", "node_relation": { "alias": "my_model", "database": "jerco", "relation_name": "\"jerco\".\"dbt_jcohen\".\"my_model\"", "schema": "dbt_jcohen" }, "node_started_at": "2023-04-10T19:21:43.288413", "node_status": "started", "resource_type": "model", "unique_id": "model.test.my_model" }, "total": 1 }, "info": { "category": "", "code": "Q011", "extra": {}, "invocation_id": "a0234d14-04ef-4eec-94bc-f409100f92e4", "level": "info", "msg": "1 of 1 START sql view model dbt_jcohen.my_model ................................ [RUN]", "name": "LogStartLine", "pid": 8576, "thread": "Thread-1", "ts": "2023-04-10T19:21:43.289185Z" } }Checklist
node_infoincludes nestednode_relationdict docs.getdbt.com#3167changie newto create a changelog entry