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

feat: transforms aggregated completion "progress" events into a new fact #1

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

pomegranited
Copy link
Member

@pomegranited pomegranited commented Jun 19, 2024

Extends https://github.com/openedx/aspects-dbt/ to add a new fact_aggregated_completions table to the reporting schema.
This new table is the same as fact_completion but also includes the results.completion field as a completed boolean.

Part of: openedx/openedx-aspects#222

@pomegranited pomegranited force-pushed the jill/transform branch 12 times, most recently from fa0f1b1 to 1e64447 Compare June 21, 2024 04:19
@pomegranited
Copy link
Member Author

pomegranited commented Jun 21, 2024

Hi @bmtcril @Ian2012 I wrote this dbt package to go along with the completion aggregator xAPI events added by open-craft/openedx-completion-aggregator#205, but I'm having trouble with the target schema: it's using reporting_xapi instead of reporting for some reason I can't see. See PR description for a screenshot.

I also tried:

Can you see where I've gone wrong?


models:
- name: fact_aggregated_completions
database: "{{ env_var('DBT_PROFILE_TARGET_DATABASE', 'reporting') }}"
Copy link

Choose a reason for hiding this comment

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

I'm seeing the same issue you are and I think this is your problem. Since Clickhouse doesn't have a concept of database, only schema when database and schema are both defined dbt_clickhouse seems to treat database as a prefix. I think if you delete this line it should work as expected.

FWIW I do also see this successfully being created in a new reporting_xapi schema and otherwise it seems to work!

Copy link

Choose a reason for hiding this comment

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

Aspects v1.0.2 solved an issue in which the environment variable for the reporting schema was not correctly set.

Copy link
Member Author

@pomegranited pomegranited Jun 25, 2024

Choose a reason for hiding this comment

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

Since Clickhouse doesn't have a concept of database, only schema when database and schema are both defined dbt_clickhouse seems to treat database as a prefix.

That's exactly what happened! I ended up deleting the schema="xapi" instead of removing database="reporting" here, because the rest of the fact_* tables are all under the reporting database, cf 8859f6a.

config(
materialized="materialized_view",
schema=env_var("ASPECTS_XAPI_DATABASE", "xapi"),
engine=aspects.get_engine("ReplacingMergeTree()"),
Copy link
Member Author

@pomegranited pomegranited Jun 25, 2024

Choose a reason for hiding this comment

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

@bmtcril FYI I also had to qualify this Aspects get_engine macro with the aspects. prefix.
Will note this in a "troubleshooting" docs PR, because this was difficult to debug :)

Copy link

Choose a reason for hiding this comment

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

Oh nice, good find 👍

@pomegranited pomegranited changed the title feat: transforms "progress" events into a new fact feat: transforms aggregated completion "progress" events into a new fact Jun 27, 2024
to use as label for subsection chart
dbt_project.yml Outdated
Copy link

Choose a reason for hiding this comment

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

For this to "extend" Aspects, you need to add a packages.yml file with:

packages:
  - git: "https://github.com/openedx/aspects-dbt.git"
    revision: vX.X.X

I'm not sure how to keep it in sync for every release of aspects-dbt.

Copy link

Choose a reason for hiding this comment

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

You can use semantic versioning, fwiw:

packages:
  - git: "https://github.com/openedx/aspects-dbt.git"
    version: [">=0.7.0", "<0.8.0"]

It's difficult to know what's a "breaking change" in dbt land, we should probably write up criteria.

Copy link
Member Author

@pomegranited pomegranited Jun 28, 2024

Choose a reason for hiding this comment

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

You can use semantic versioning, fwiw:

Nope, dbt doesn't like that, raises a Validator error (docs)

I'm not sure how to keep it in sync for every release of aspects-dbt.

Maybe there's a tutor templated way? But I think it can be manual for now.

Copy link

Choose a reason for hiding this comment

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

We could inject it, but @bmtcril wanted to avoid introducing breaking changes

@pomegranited pomegranited merged commit d1368ad into main Jul 2, 2024
@pomegranited pomegranited deleted the jill/transform branch July 2, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants