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.

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

[CT-65] [Bug] Metrics' names shouldn’t be allowed to contain spaces #4572

Closed
1 task done
joellabes opened this issue Jan 14, 2022 · 3 comments · Fixed by #5173
Closed
1 task done

[CT-65] [Bug] Metrics' names shouldn’t be allowed to contain spaces #4572

joellabes opened this issue Jan 14, 2022 · 3 comments · Fixed by #5173
Assignees
Labels
bug Something isn't working jira

Comments

@joellabes
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

I'm working on macros to generate SQL to calculate metrics, and want to be able to use metric.name as the column alias. I just discovered that they're allowed to contain spaces - I don't know if this was intentional or not considered, but I think it's bad when there's a human-readable label available as well.

Expected Behavior

slack users to be rejected because it's not [_A-Za-z] (or maybe even just [_a-z]?)

Steps To Reproduce

metrics:
    - name: slack joiners
      label: Members of Community Slack
      model: dim_slack_users_2

Relevant log output

No response

Environment

- dbt: 1.0.0

What database are you using dbt with?

snowflake

Additional Context

No response

@joellabes joellabes added bug Something isn't working triage labels Jan 14, 2022
@jtcohen6 jtcohen6 added this to the v1.0.2 milestone Jan 14, 2022
@gshank gshank self-assigned this Jan 17, 2022
@gshank
Copy link
Contributor

gshank commented Jan 17, 2022

I think that this was an oversight. We'll discuss what we'd like to accept for the metrics name and fix this.

@gshank gshank removed the triage label Jan 17, 2022
@gshank gshank removed their assignment Jan 17, 2022
@leahwicz leahwicz added the jira label Jan 19, 2022
@leahwicz leahwicz changed the title [Bug] Metrics' names shouldn’t be allowed to contain spaces [CT-65][Bug] Metrics' names shouldn’t be allowed to contain spaces Jan 19, 2022
@leahwicz leahwicz changed the title [CT-65][Bug] Metrics' names shouldn’t be allowed to contain spaces [CT-65] [Bug] Metrics' names shouldn’t be allowed to contain spaces Jan 19, 2022
@jtcohen6 jtcohen6 modified the milestones: v1.0.2, v1.0.3 Feb 2, 2022
@jtcohen6 jtcohen6 modified the milestones: v1.0.3, v1.1 Feb 14, 2022
@leahwicz
Copy link
Contributor

@jtcohen6 is the exit criteria here just to prohibit spaces in metric names?

@joellabes
Copy link
Contributor Author

My preference would be to constrain it pretty severely - right now I'm passing the metric name directly in as a column alias. For example, even the metrics that our internal data team has made will be problematic, cos they're using periods:

  - name: okr_1.2.0__new_verified_accounts_activated
    label: New Verified Accounts Activated
    model: ref('fct_cloud_accounts')
    description: '{{ doc("okr_new_verified_accounts_activated") }}'
...

Ideally it should be just stuff that's legal in a column name (which also has implications around whether a metric's name can start with a digit).

Alternatively, I can tidy up the column alias myself, but that could cause conflicts with any low-code tools built on top of the metrics package that will need to know how to find the resulting column.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working jira
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants