Ele 1484 source freshness invocation by NoyaArie · Pull Request #1165 · elementary-data/elementary

NoyaArie · 2023-09-18T11:01:52Z

No description provided.

linear · 2023-09-18T11:01:54Z

Is your feature request related to a problem? Please describe.
Trying to cross-reference the data on model dbt_source_freshness_results and the respective jobs (on BigQuery audit logs), I've realized that the invocation_id on this table relates to the invocation of the upload_source_freshness macro and that the invocation_id for the source freshness invocation is not stored.
Also, it is a little confusing to have the upload_source_freshness invocation_id concatenated to make the source_freshness_execution_id, since it is not the source freshness execution, but a whole different job.

Describe the solution you'd like

Including a new column on the dbt_source_freshness_results model with the source freshness execution invocation_id.
Using the source freshness execution invocation_id to create the source_freshness_execution_id field

Describe alternatives you've considered
No alternatives, since the invocation_id of the source freshness execution is not included anywhere

Additional context

The requested source freshness execution invocation_id is available on the artifact target/sources.json where all the other data is extracted from, so it shouldn't be a big challenge to include it.
There are concerns with creating a clear nomenclature if the currently stored upload_source_freshness invocation_id is to be kept, since two different invocation ids would be stored on this table.
Adding a new column would change the model schema and require recreating tables.

Would you be willing to contribute this feature?
Yes, I can contribute this.

github-actions · 2023-09-18T11:02:02Z

👋 @NoyaArie
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

NoyaArie · 2023-09-18T11:08:56Z

This PR also covers upload-once-source-freshness-results

elongl · 2023-09-19T07:25:05Z

+        )
+        select count(*) as count
+        from invocations
+        where invocation_id = {{ "'" ~ invocation_id ~ "'" }}


Suggested change

where invocation_id = {{ "'" ~ invocation_id ~ "'" }}

where invocation_id = {{ elementary.edr_quote(invocation_id) }}

elongl · 2023-09-19T07:27:38Z

@@ -1,5 +1,5 @@
 {% macro upload_source_freshness(results) %}
-  {% set source_freshness_results_dicts = fromjson(results) %}
+  {% set source_freshness_results_dict = fromjson(results) %}


I think this does take multiple dicts though as target/sources.json:results is a list.
Otherwise it wouldn't work since upload_artifacts_to_table takes a list.

renaming to source_freshness_results :)

elongl · 2023-09-19T07:28:33Z

+        where invocation_id = {{ "'" ~ invocation_id ~ "'" }}
+    {% endset %}
+
+    {% set result = elementary.run_query(query) %}


Maybe use .result_value()?

elongl · 2023-09-19T07:29:56Z

-        self.upload_results(results)
-        click.echo("Uploaded source freshness results successfully.")
+        sources_file_contents = self.get_sources_file_contents()
+        if sources_file_contents:


How could it be falsely?

empty file, which should not happen, but since in the next line accessing to keys prefer checking it

Personally I would just remove it as this is not something that can happen and this flow is a bit unclear.
If you want to keep it, maybe do:

if not sources_file_contents: raise raise click.ClickException("Unexpectedly found an empty source.json file.")

elongl · 2023-09-19T07:31:22Z

@@ -0,0 +1,19 @@
+{% macro validate_source_freshness_invocation(invocation_id, days_back=14) %}


Care to rename this macro to something that indicates that it checks whether the source freshness was uploaded or not?

renaming the macro to can_upload_source_freshness, and the internal variables as well

elongl · 2023-09-19T08:46:50Z

-        self.upload_results(results)
-        click.echo("Uploaded source freshness results successfully.")
+        sources_file_contents = self.get_sources_file_contents()
+        if sources_file_contents:


Personally I would just remove it as this is not something that can happen and this flow is a bit unclear.
If you want to keep it, maybe do:

if not sources_file_contents: raise raise click.ClickException("Unexpectedly found an empty source.json file.")

elongl · 2023-09-19T08:47:47Z

        for chunk in upload_with_progress_bar:
            results_segment = results[chunk : chunk + chunk_size]
+
+            for result in results_segment:


What's the purpose of this?

need to pass the metadata to flatten_source_freshness for each record

NoyaArie mentioned this pull request Sep 18, 2023

Ele 1484 source freshness invocation elementary-data/dbt-data-reliability#537

Merged

elongl reviewed Sep 19, 2023

View reviewed changes

NoyaArie added 3 commits September 19, 2023 11:00

fix typo

89898e9

add metadata

dc4113c

validate invocation id

a9447b1

NoyaArie force-pushed the ele-1484-source-freshness-invocation-id branch from c48b93f to a9447b1 Compare September 19, 2023 08:00

renaming and refactor

b10ff1e

elongl reviewed Sep 19, 2023

View reviewed changes

remove falsy condition

ce66ee8

elongl approved these changes Sep 19, 2023

View reviewed changes

Merge branch 'master' into ele-1484-source-freshness-invocation-id

0bf606f

NoyaArie merged commit c9f9996 into master Sep 19, 2023

NoyaArie deleted the ele-1484-source-freshness-invocation-id branch September 19, 2023 15:03

	where invocation_id = {{ "'" ~ invocation_id ~ "'" }}
	where invocation_id = {{ elementary.edr_quote(invocation_id) }}

		@@ -0,0 +1,19 @@
		{% macro validate_source_freshness_invocation(invocation_id, days_back=14) %}

Conversation

NoyaArie commented Sep 18, 2023

Uh oh!

linear Bot commented Sep 18, 2023

Uh oh!

github-actions Bot commented Sep 18, 2023

Uh oh!

NoyaArie commented Sep 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants