Update ingestion to write feature metrics for validation #449

davidheryanto · 2020-01-29T01:49:43Z

What this PR does / why we need it:
This PR updates Feast Ingestion to write feature level metrics so feature values and presence can be validated. The schema for feature validation follows that from Tensorflow metadata https://github.com/tensorflow/metadata/blob/master/tensorflow_metadata/proto/v0/schema.proto. The FeatureSpec and EntitySpec in FeatureSet.proto has been updated to partially support the validation schema in #438.

This PR only addresses the Milestone 1 requirements in https://docs.google.com/document/d/1TPmd7r4mniL9Y-V_glZaWNo5LMXLshEAUpYsohojZ-8

In order to fully make use of these metrics, it is assumed that:

StatsD collector is running so Feast ingestion job can push the feature level metrics via StatsD
A StatsD to Prometheus exporter is running to convert StatsD metrics to Prometheus (This is because other metrics in Feast, makes use Prometheus, so the conversion is to standardize the metrics type. We use StatsD in ingestion job because of the pull based behaviour of Prometheus https://cloud.google.com/blog/products/data-analytics/managing-and-monitoring-a-cloud-dataflow-setup)
The prometheus metrics are scraped regularly

A sample grafana-dashboard.json is also provided for a generic Grafana dasboard that users can import to use the Prometheus metrics. The alerts can then be configured accordingly. Sample dashboard looks like the following:

NOTE that FeaturePresence https://github.com/tensorflow/metadata/blob/master/tensorflow_metadata/proto/v0/schema.proto#L544 defined in the Tensorflow metadata is used in the batch use case in the Tensorflow example. The scope for feature counts and fractions are all the features in a particular batch ingestion. In the continuous ingestion case having an unbounded source, however, the scope needs to be specified by the user. In the sample Grafana dashboard provided above, this scope is defined by the range selected by the user (the top right Last 5 minutes)

Which issue(s) this PR fixes:
Partially addresses #172

Does this PR introduce a user-facing change?:

NONE

- Ignore StatsD execption since it is not gonna be thrown when using non blocking StatsD client: https://github.com/DataDog/java-dogstatsd-client#unix-domain-socket-support - Reuse tags variables - Add validation for correct feature set reference in FeatureRow

And make the checking of exporter type in ingestion more robust.

…ata schema

Previosly missing the word 'ingestion'

feast-ci-bot · 2020-01-29T01:49:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidheryanto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [davidheryanto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ingestion/src/main/java/feast/ingestion/transform/metrics/WriteMetricsTransform.java

davidheryanto · 2020-01-29T04:20:07Z

/hold
I will add tests by calling Prometeheus API in test-end-to-end.sh stage to ensure that metrics are as expected.

Yanson · 2020-01-29T11:44:47Z

A StatsD to Prometheus exporter is running to convert StatsD metrics to Prometheus (This is because other metrics in Feast, makes use Prometheus, so the conversion is to standardize the metrics type. We use StatsD in ingestion job because of the pull based behaviour of Prometheus https://cloud.google.com/blog/products/data-analytics/managing-and-monitoring-a-cloud-dataflow-setup)

Can this (and any other metrics in ingestion) be migrated to standard Beam custom metrics?

https://beam.apache.org/releases/javadoc/2.1.0/org/apache/beam/sdk/metrics/Metrics.html

These will automatically end up in Stackdriver (when using Dataflow) and you can use a Stackdriver exporter for Prometheus:

https://github.com/helm/charts/tree/master/stable/stackdriver-exporter

woop · 2020-01-29T14:32:37Z

@davidheryanto In addition to the test, can we please also add documentation to describe to users how they can benefit from this? Specifically

The configuration options they have available
Setting up the metric collection environment
The metrics that can be collected and possible examples of them (as you have above)

woop · 2020-01-29T14:34:51Z

A StatsD to Prometheus exporter is running to convert StatsD metrics to Prometheus (This is because other metrics in Feast, makes use Prometheus, so the conversion is to standardize the metrics type. We use StatsD in ingestion job because of the pull based behaviour of Prometheus https://cloud.google.com/blog/products/data-analytics/managing-and-monitoring-a-cloud-dataflow-setup)

Can this (and any other metrics in ingestion) be migrated to standard Beam custom metrics?

https://beam.apache.org/releases/javadoc/2.1.0/org/apache/beam/sdk/metrics/Metrics.html

These will automatically end up in Stackdriver (when using Dataflow) and you can use a Stackdriver exporter for Prometheus:

https://github.com/helm/charts/tree/master/stable/stackdriver-exporter

Hi @Yanson, thanks for you input. I will let @davidheryanto and @zhilingc respond to you on this point in more detail, but we previously did use Beam custom metrics but I believe we moved away due to its metrics being scoped to a global window and the limited types of instrumentation compared to statsd/prom.

zhilingc · 2020-01-30T01:32:42Z

A StatsD to Prometheus exporter is running to convert StatsD metrics to Prometheus (This is because other metrics in Feast, makes use Prometheus, so the conversion is to standardize the metrics type. We use StatsD in ingestion job because of the pull based behaviour of Prometheus https://cloud.google.com/blog/products/data-analytics/managing-and-monitoring-a-cloud-dataflow-setup)

Can this (and any other metrics in ingestion) be migrated to standard Beam custom metrics?

https://beam.apache.org/releases/javadoc/2.1.0/org/apache/beam/sdk/metrics/Metrics.html

These will automatically end up in Stackdriver (when using Dataflow) and you can use a Stackdriver exporter for Prometheus:

https://github.com/helm/charts/tree/master/stable/stackdriver-exporter

Thanks for the suggestion! I think its a viable option. Beam metrics behave very similarly to prometheus exporters, and as long as its exported out to prom (which can compute time windowed metrics using the delta) it should work just fine.

- Update Field, Feature and Entity class with fields from presence_constraints, shape_type and domain_info

- Create 2 distinct services, one for metrics, one for statsd because they use different protocols (TCP and UDP) respectively and exposing this via Kube LoadBalancer won't work by default because most LoadBalancer only supports either TCP or UDP - Allow users to define statsd_mapping from helm values

- Include more filters for constraint validation (so no many to many results for subtrations) - Use datasource: null so dashboard will use "default" datasource - Unset uid so it will be autogenerated during import

davidheryanto · 2020-02-10T09:08:35Z

/hold cancel

davidheryanto · 2020-02-11T02:30:28Z

/retest

woop · 2020-02-11T02:31:19Z

@Yanson I have created a separate issue to track an evaluation of Beam metrics to replace Statsd: #469

woop · 2020-02-11T02:31:41Z

@davidheryanto waiting for the tests to pass before I evaluate this PR

woop · 2020-02-11T02:44:41Z

examples/data_validation/working_with_schema.ipynb

+    "%%bash\n",
+    "# Sample data from BigQuery public dataset: bikeshare stations\n",
+    "# https://cloud.google.com/bigquery/public-data\n",
+    "wget https://raw.githubusercontent.com/davidheryanto/feast/update-ingestion-metrics-for-validation/examples/data_validation/bikeshare_stations.csv\n",


davidheryanto?

yes because this PR is not merged yet. I will update it once merged

woop · 2020-02-11T02:44:56Z

infra/charts/feast/charts/feast-core/values.yaml

+  dashboards:
+    feast:
+      feast-ingestion-dashboard:
+        url: https://raw.githubusercontent.com/davidheryanto/feast/update-ingestion-metrics-for-validation/ingestion/src/main/resources/grafana-dashboard.json


davidheryanto again

woop · 2020-02-11T02:48:45Z

.prow/scripts/test-end-to-end.sh

 sleep 20
 tail -n10 /var/log/kafka.log
 kafkacat -b localhost:9092 -L

+echo "
+============================================================
+Installing Telegraf with StatsD input and Prometheus output


This needs to be changed to statsd_exporter as below.

woop · 2020-02-11T04:29:30Z

infra/charts/feast/charts/feast-core/requirements.yaml

+- name: prometheus-statsd-exporter
+  version: 0.1.2
+  condition: prometheus-statsd-exporter.enabled


This should be moved out of core.

woop · 2020-02-11T04:36:00Z

sdk/python/feast/field.py

+
+    @presence.setter
+    def presence(self, presence: schema_pb2.FeaturePresence):
+        if not isinstance(presence, schema_pb2.FeaturePresence):


Should we add comments for the setters?

woop · 2020-02-11T04:36:18Z

sdk/python/feast/field.py

+            self.value_count = feature.value_count
+
+    def update_domain_info(
+        self, feature: schema_pb2.Feature, schema: schema_pb2.Schema = None


More missing comments

woop · 2020-02-11T04:37:47Z

sdk/python/tests/test_feature_set.py

@@ -167,3 +170,94 @@ def test_add_features_from_df_success(
        )
        assert len(my_feature_set.features) == feature_count
        assert len(my_feature_set.entities) == entity_count
+
+    def test_import_tfx_schema(self):
+        tests_folder = pathlib.Path(__file__).parent


Doesn't this depend on where pytest is launched from?

Oh pathlib.Path(__file__) will give the correct relative path to test_feature_set.py. If pytest is called from different different context, the relative path will change to the correct location too.

woop · 2020-02-11T04:38:59Z

sdk/python/tests/test_feature_set.py

@@ -11,21 +11,24 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.


Shouldn't we also add the other missing tests like for your update methods?

Ok will add it

davidheryanto · 2020-02-11T10:55:00Z

/hold
Updating charts

woop · 2020-02-12T08:50:23Z

This PR is still missing
A fixed number of histogram bins are generated from the bounds provided by the user
based on the Feature Validation RFC

davidheryanto · 2020-03-05T02:12:28Z

Closing this because the master has been updated to include writing metrics for feature values, which override with the changes here.
And will break the changes to smaller parts e.g. exclude the updates to Helm chart etc.

ches · 2020-03-08T19:35:17Z

I believe the last comment refers to #486, for reference.

davidheryanto added 11 commits January 22, 2020 03:14

Refactor map variable for feature set ref to feature set object

d25f77d

Update documentation for FeatureRow.proto

cbb720b

Add docs to ImportOptions for metrics exporter type.

3059bc4

And make the checking of exporter type in ingestion more robust.

Write value and constraint metrics for each feature

13bc909

Add grafana-dashboard.json for features validation

2e44197

Make fixed window size a part of pipeline options

6fb8725

Merge branch 'master' into update-ingestion-metrics-for-validation

d2f74e5

Update generated protobuf code for Python to include Tensorflow metad…

ec2e6c8

…ata schema

Add skeleton for update/get schema in FeatureSet

539cd1d

Update tag name for feast ingestion job

dcbf9c1

Previosly missing the word 'ingestion'

davidheryanto requested review from khorshuheng, pradithya, woop and zhilingc as code owners January 29, 2020 01:49

feast-ci-bot added approved size/XXL labels Jan 29, 2020

davidheryanto changed the title ~~Update ingestion metrics for validation~~ Update ingestion to write Feature metrics for validation Jan 29, 2020

davidheryanto changed the title ~~Update ingestion to write Feature metrics for validation~~ Update ingestion to write feature metrics for validation Jan 29, 2020

woop reviewed Jan 29, 2020

View reviewed changes

ingestion/src/main/java/feast/ingestion/transform/metrics/WriteMetricsTransform.java Show resolved Hide resolved

feast-ci-bot added the do-not-merge/hold label Jan 29, 2020

davidheryanto added 3 commits January 30, 2020 13:08

Add update_schema method to FeatureSet

d33c546

- Update Field, Feature and Entity class with fields from presence_constraints, shape_type and domain_info

Update error message when domain ref is missing from top level schema

08654c4

Add more assertion in test_update_schema before updating schema

c3b68f7

davidheryanto force-pushed the update-ingestion-metrics-for-validation branch from 27f43fd to 2887e8c Compare February 9, 2020 14:09

Update grafana-dashboard for ingestion

0b72ddd

- Include more filters for constraint validation (so no many to many results for subtrations) - Use datasource: null so dashboard will use "default" datasource - Unset uid so it will be autogenerated during import

davidheryanto force-pushed the update-ingestion-metrics-for-validation branch from 2887e8c to 0b72ddd Compare February 9, 2020 14:35

davidheryanto added 2 commits February 10, 2020 02:54

Add sample csv for data validation

1fe767a

Add sample notebook for working with schema for data validation

5593040

davidheryanto force-pushed the update-ingestion-metrics-for-validation branch from f179240 to 5593040 Compare February 10, 2020 01:36

davidheryanto added 2 commits February 10, 2020 09:38

Update validation schema in test dataset

1708373

Add grafana and prometheus dependency in feast-core

861f21d

davidheryanto force-pushed the update-ingestion-metrics-for-validation branch 2 times, most recently from acbe497 to c4d66e7 Compare February 10, 2020 08:57

Add statsdexporter, prometheus and grafana to docker-compose

1a6ebc5

davidheryanto force-pushed the update-ingestion-metrics-for-validation branch from c4d66e7 to 1a6ebc5 Compare February 10, 2020 08:59

feast-ci-bot removed the do-not-merge/hold label Feb 10, 2020

ApplyFeatureSet should update FeatureSet when constraints are updated

63809f5

woop mentioned this pull request Feb 11, 2020

Evaluate the possibility of using Beam metrics #469

Closed

woop requested changes Feb 11, 2020

View reviewed changes

woop mentioned this pull request Feb 11, 2020

Update Python SDK so FeatureSet can import Schema from Tensorflow metadata #450

Merged

Merge branch 'master' into update-ingestion-metrics-for-validation

467fbdc

feast-ci-bot added the do-not-merge/hold label Feb 11, 2020

ches added this to the v0.5.0 milestone Feb 14, 2020

davidheryanto closed this Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ingestion to write feature metrics for validation #449

Update ingestion to write feature metrics for validation #449

davidheryanto commented Jan 29, 2020 •

edited

Loading

feast-ci-bot commented Jan 29, 2020

davidheryanto commented Jan 29, 2020

Yanson commented Jan 29, 2020

woop commented Jan 29, 2020

woop commented Jan 29, 2020 •

edited

Loading

zhilingc commented Jan 30, 2020

davidheryanto commented Feb 10, 2020

davidheryanto commented Feb 11, 2020

woop commented Feb 11, 2020

woop commented Feb 11, 2020

woop Feb 11, 2020

davidheryanto Feb 11, 2020

woop Feb 11, 2020

woop Feb 11, 2020

woop Feb 11, 2020

woop Feb 11, 2020

woop Feb 11, 2020

woop Feb 11, 2020

davidheryanto Feb 11, 2020

woop Feb 11, 2020

davidheryanto Feb 11, 2020

davidheryanto commented Feb 11, 2020

woop commented Feb 12, 2020

davidheryanto commented Mar 5, 2020

ches commented Mar 8, 2020

		@@ -11,21 +11,24 @@
		# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

Update ingestion to write feature metrics for validation #449

Update ingestion to write feature metrics for validation #449

Conversation

davidheryanto commented Jan 29, 2020 • edited Loading

feast-ci-bot commented Jan 29, 2020

davidheryanto commented Jan 29, 2020

Yanson commented Jan 29, 2020

woop commented Jan 29, 2020

woop commented Jan 29, 2020 • edited Loading

zhilingc commented Jan 30, 2020

davidheryanto commented Feb 10, 2020

davidheryanto commented Feb 11, 2020

woop commented Feb 11, 2020

woop commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidheryanto commented Feb 11, 2020

woop commented Feb 12, 2020

davidheryanto commented Mar 5, 2020

ches commented Mar 8, 2020

davidheryanto commented Jan 29, 2020 •

edited

Loading

woop commented Jan 29, 2020 •

edited

Loading