feat: add metadata models #19
Conversation
ravisuhag
requested review from
AkarshSatija,
kushsharma,
rahmatrhd,
rohilsurana and
sravankorumilli
odpf/meta/facets/Columns.proto
Outdated
| message Column { | ||
| string name = 1; | ||
| string description = 2; | ||
| string data_type = 3; |
There was a problem hiding this comment.
Column.type might mean something else while Column.data_type clearly talking about the data type as invarchar or int.
wdyt?
There was a problem hiding this comment.
Sounds good 👍 but then we should follow this in length field as well right?
There was a problem hiding this comment.
imo column length is pretty common but maybe it's just me and if you didnt catch it the first time probably it needs to be renamed.
what would be clearer, field_length, data_length?
odpf/meta/facets/Custom.proto
Outdated
| option go_package = "github.com/odpf/proton/meta/facets"; | ||
|
|
||
| message Custom { | ||
| map<string,string> custom_properties = 1; |
There was a problem hiding this comment.
just call it properties
Also I can imagine this could contain key -> []string use cases. I think its better if we use Value here
odpf/meta/Chart.proto
Outdated
| string source = 4; | ||
| string description = 5; | ||
| string url = 6; | ||
| string raw_query = 7; |
There was a problem hiding this comment.
For Grafana, there is a possibility of more than one query. Need to consider that as well. Suggestion, we can use array of string.
There was a problem hiding this comment.
Also there is a need to know for which data source the query is run against. For example, in Grafana, a panel can have multiple queries, but the data source should only be one.
There was a problem hiding this comment.
Also there is a need to know for which data source the query is run against. For example, in Grafana, a panel can have multiple queries, but the data source should only be one.
added string data_source on chart to solve this.
For Grafana, there is a possibility of more than one query. Need to consider that as well. Suggestion, we can use array of string.
yeah agree, we need to collect more dashboard sample first though to make sure the changes would be generic.
for example, atm metabase card only has one query.
for now I would suggest adding extra queries to custom facets e.g. raw_query_2. Once we see a lot of chart has multiple queries then we can remodel it.
But I would assume most time-series dashboard would have multiple queries. 🤔
ravisuhag
force-pushed
the
metadata-model
branch
from
555a23f to
5f2f7b2
Compare
ravisuhag
force-pushed
the
metadata-model
branch
from
5f2f7b2 to
b90502d
Compare
StewartJingga
left a comment
StewartJingga
left a comment
There was a problem hiding this comment.
the new folder structure is odpf/entities/[common,facets,resources].
do you think we should somehow mention metadata somewhere?
odpf/entities might mean all entities/models for odpf instead of metadata.
Ideally, I was thinking of actually having all entities of odpf but only from a metadata point of view. Let me think if I can add that context somehow. |
No description provided.