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

Added a required 'name' argument to 'collect_metrics'. #736

Merged

Conversation

elongl
Copy link
Member

@elongl elongl commented Jul 16, 2024

No description provided.

Copy link

linear bot commented Jul 16, 2024

Copy link
Contributor

👋 @elongl
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 the elementary repository.

@elongl elongl closed this Jul 21, 2024
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch from e2e4310 to 8572db5 Compare July 21, 2024 08:39
@elongl elongl reopened this Jul 21, 2024
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch from 55f0585 to 4757a6c Compare July 21, 2024 11:14
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch 3 times, most recently from 6bcd126 to 0449107 Compare July 22, 2024 13:43
{% for metric in column_metrics %}
{% do metric_types.append(metric.type) %}
{% do metric_name_to_type.update({metric.name: metric.type}) %}
{% endfor %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this is the right place, but can we also save metric_type in data_monitoring_metrics?
(not sure what's the right place for this comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

So my intuition was to do that as well,
and Ofek convinced me that it shouldn't matter how a metric is calculated for the storage of the metric. Regarding which algorithm to use in the cloud, we can view the type in dbt_tests at the configuration. WDYT?

Copy link
Collaborator

@haritamar haritamar Jul 25, 2024

Choose a reason for hiding this comment

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

Hmm the only case where I think it might matter is when the metric definition is changed - e.g. the name remains the same but the metric type is changed.
But that makes me think that it should actually be added to metric_properties. WDYT?

This achieves two things:

  1. The metric will be backfilled if the type changes (same as any other configuration change like timestamp_column)
  2. In the cloud I implemented logic that only pulls metrics with the most recent metric properties (to ensure we only rely on metrics collected with the most recent configuration - a bit like what happens in get_anomaly_scores_query).

@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch 3 times, most recently from 0603fc8 to 2fa0bfa Compare July 25, 2024 12:54
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch from 2fa0bfa to c8ae4ad Compare July 25, 2024 12:55
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch from 12a7cc9 to 8dc050e Compare July 29, 2024 08:34
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch 4 times, most recently from 6a232f5 to e997fc3 Compare July 29, 2024 10:07
@elongl elongl force-pushed the ele-3405-change-collect_metrics-to-require-a-name-argument branch from e997fc3 to f9a2f8f Compare July 29, 2024 10:34
@elongl elongl merged commit 6e4789f into master Jul 29, 2024
12 checks passed
@elongl elongl deleted the ele-3405-change-collect_metrics-to-require-a-name-argument branch July 29, 2024 11:35
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.

2 participants