Skip to content
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.

Sign up for GitHub

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

Add label, type, and SQL to metric docs #278

Closed
wants to merge 5 commits into from
Closed

Add label, type, and SQL to metric docs #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c7284d
Add label, type, and SQL to metric docs
GtheSheep Jun 8, 2022
b2dfe86
Update CHANGELOG.md
GtheSheep Jun 8, 2022
4ab30c9
fix - don't override metric label
GtheSheep Jun 30, 2022
427636d
Merge branch 'dbt-labs:main' into main
GtheSheep Jun 30, 2022
b834b59
fix - suggested changes
GtheSheep Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
- [Snyk] Upgrade prismjs from 1.27.0 to 1.28.0 ([#271](https://github.com/dbt-labs/dbt-docs/issues/271))
- Fixed sample SQL Code for sources when no database is defined ([docs#272](https://github.com/dbt-labs/dbt-docs/pull/272))
- Run build and tests in CI checks ([docs#274](https://github.com/dbt-labs/dbt-docs/pull/274))
- Add label, type, and SQL to metric docs ([docs#278](https://github.com/dbt-labs/dbt-docs/pull/278))

Contributors:
- [@b-per](https://github.com/b-per) ([docs#272](https://github.com/dbt-labs/dbt-docs/pull/272))
- [@gthesheep](https://github.com/gthesheep) ([docs#278](https://github.com/dbt-labs/dbt-docs/pull/278))

## dbt-core 1.1.0 (April 28, 2022)
- Fixed capitalization in UI for exposures of `type: ml` ([#256](https://github.com/dbt-labs/dbt-docs/issues/256))
Expand Down
15 changes: 14 additions & 1 deletion src/app/docs/metric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ angular
$scope.project = projectService;

$scope.codeService = codeService;
$scope.extra_table_fields = [];
$scope.versions = {};

$scope.metric = {};
Expand All @@ -22,6 +21,20 @@ angular
$scope.metric = metric;
$scope.parents = dag_utils.getParents(project, metric);
$scope.parentsLength = $scope.parents.length;
$scope.extra_table_fields = [
{
name: "Label",
value: $scope.metric.metric_label,
},
{
name: "Type",
value: $scope.metric.type,
},
{
name: "SQL",
value: $scope.metric.sql,
},
];

})
}]);
2 changes: 2 additions & 0 deletions src/app/services/project_service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ angular

// Add metrics back into nodes to make site logic work
_.each(service.files.manifest.metrics, function(node) {
// node.label is reserved for use in the DAG so redefine the label here so we can access it in metric.js before assinging the value os `node.name` to `node.label`
node.metric_label = node.label;
node.label = node.name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually has to stay here as node.label is what is used in the DAG. Removing this line results in the metric label, as defined in the yaml file in the project, showing on the DAG instead of the metric name, as desired.

This is the result of a design choice to use label on the DAG. I do not think it's necessary to rework all this logic for the name in this case. I would just change this to

// node.label is reserved for use in the DAG so redefine the label here so we can access it in metric.js before assinging the value os `node.name` to `node.label`
node.metric_label = node.label;
node.label = node.name;

And then making the equivalent change over in metric.js of:

{
    name: "Label",
    value: $scope.metric.metric_label,
},

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was exactly the kind of thing I was worried about 😹 but yeah sure, will update 👌

Copy link
Member

@emmyoop emmyoop Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GtheSheep don't update it yet! We're having a conversation literally right now about this. Will get back to you in 5 min!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @GtheSheep - thanks very much for digging into this! @callum-mcdata and I did something similar in #285 -- I wish i had saw this PR first!!

We debated internally about showing the metric label instead of the metric name in the DAG viz (and in other parts of the UI). My thinking is that it's preferable to see the label (say: Average Revenue Per Customer) instead of a less human-readable name (eg. arpc). What do you think makes more sense / you'd prefer to see?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @drewbanin - yeah I think it most cases it's probably preferable to see the label, even if that's not quite consistent with some of the other objects, but if it's just a styling choice with no other impacts it's probably nicer for general users, the only slight downside I can see is the lineage graph then not matching the selector at the bottom, but that seems minimal 🤷‍♂️

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to close this in favour of #285 😸

service.files.manifest.nodes[node.unique_id] = node;
});
Expand Down