Add label, type, and SQL to metric docs

[GtheSheep]

resolves #246

Description

Adds metric label, type, and SQL to their docs pages under the extra fields in details, not sure if that's the desired placement for the SQL though?

Checklist

• I have signed the CLA
• I have generated docs locally, and this change appears to resolve the stated issue
• I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

[emmyoop]

@GtheSheep thanks so much for the contribution!

I got some time to test this out and it looks like the metric name is actually what ends up in the metric label field. It's a head scratcher. I'm not seeing anything obvious in the code or the manifest.json to make me think this would happen. We will need to get that fixed up before we can get this change merged in but wanted to drop you a note to let you know this PR is on our radar!

In the meantime I'm going to work to get the failing test that is not related to your change fixed up so that the tests will pass after you merge my (soon to come) changes in.

[GtheSheep]

@emmyoop ahh thanks, yeah that error looked a little confusing 😅
That's very odd, maybe it's to do with this line? https://github.com/dbt-labs/dbt-docs/blob/main/src/app/services/project_service.js#L151

[GtheSheep]

Yeah, that did it, if I set:

node.Label = node.label;
node.label = node.name;

then choose to display node.Label, it shows the metric label correctly, though this is a bit hacky, and I don't know enough about this project to know what not overriding node.label with node.name might impact 😅 though if I remove the node.label = node.name; line and continue with what's here, it does seem fine when serving docs

[emmyoop]

The error in building should be resolved now if you pull in the latests from dbt-docs into your fork.

I've requested one small tweak and then this should be good to merge in. Thanks for your work on it!

[emmyoop]

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,
},

[GtheSheep]

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

[emmyoop]

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

[GtheSheep]

thank you!

[drewbanin]

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?

[GtheSheep]

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 🤷‍♂️

[GtheSheep]

happy to close this in favour of #285 😸

[emmyoop]

Closing in favor of #285