chore: Reorganize feature logging in Go for better extensibility by pyalex · Pull Request #2614 · feast-dev/feast

pyalex · 2022-04-26T23:03:53Z

What this PR does / why we need it:

This is mostly refactoring PR. This refactoring is needed before integrating Python API for feature logging in offline store.
Improvements:

Splitting LoggingService (single instance) with Logger (one per feature service) - allows to cache logger (per feature service) instead of creating new one on each request.
Decoupling schema generation from logger - improves testing.
Decoupling buffer operations - improves testing.
Simplifying sink logic - there will be one prod and one test sink in foreseeable future, it's better to keep things simple - improves testing.

Which issue(s) this PR fixes:

Fixes #

codecov-commenter · 2022-04-26T23:09:43Z

Codecov Report

Merging #2614 (dd563fb) into master (f3ff812) will decrease coverage by 0.04%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #2614      +/-   ##
==========================================
- Coverage   81.64%   81.60%   -0.05%     
==========================================
  Files         161      161              
  Lines       13304    13307       +3     
==========================================
- Hits        10862    10859       -3     
- Misses       2442     2448       +6

Flag	Coverage Δ
integrationtests	`72.56% <60.00%> (-0.34%)`	⬇️
unittests	`59.39% <40.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdk/python/feast/feature_logging.py	`77.77% <60.00%> (-0.39%)`	⬇️
sdk/python/tests/utils/online_read_write_test.py	`93.54% <0.00%> (-6.46%)`	⬇️
.../integration/online_store/test_online_retrieval.py	`96.84% <0.00%> (-3.16%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3ff812...dd563fb. Read the comment docs.

.gitignore

kevjumba · 2022-04-26T23:51:50Z

go/cmd/server/server_test.go

Do we no longer need to clean up the generated log file?

Yep, it's written to temp directory, which will be cleaned up by test framework

kevjumba · 2022-04-26T23:53:48Z

go/internal/feast/logging/featureserviceschema.go

I think this is a comment that is no longer needed as we have tests now.

kevjumba · 2022-04-26T23:55:07Z

go/internal/feast/logging/filelogsink.go

Remove comment.

kevjumba · 2022-04-26T23:55:21Z

go/internal/feast/logging/filelogsink.go

Remove comment.

kevjumba · 2022-04-27T00:13:40Z

go/internal/feast/logging/logger_test.go

remove comment.

kevjumba · 2022-04-27T00:22:01Z

go/internal/test/go_integration_test_utils.go

Are these not supported types?

Yeah, there're Timestamp_ms and Date32 types that are not convertible to proto. We write Timestamp_s to parquet but somehow it's being read by Go parquet reader as Timestamp_ms. Reasons are unclear yet. Date32 is used system column log_date. We can skip it for now in tests.

kevjumba · 2022-04-27T06:10:17Z

go/internal/feast/logging/filelogsink.go

file log sink?

kevjumba · 2022-04-27T06:13:04Z

go/internal/feast/logging/logger.go

I think this should also have error handling for writing batch.

We cannot really handle this error. But added printing

kevjumba · 2022-04-27T06:13:56Z

go/internal/feast/logging/logger.go

Maybe add some comments here that specify that differentiates flushing from writing.

Added comments to LogSink interface

achals · 2022-04-27T16:38:43Z

go/internal/feast/logging/service.go

I think there should be comments explaining all of these fields

achals · 2022-04-27T16:39:31Z

go/internal/feast/model/featureservice.go

Whats a PartitionInterval for?

Not used right now. Deleted

achals · 2022-04-27T16:43:31Z

sdk/python/feast/feature_logging.py

Shouldn't the default sample rate be 0 for perf?

The default behaviour is omission of LoggingConfig in feature service. In this case no logging happening. But if you added logging config then default sample_rate is 100%.

go/internal/feast/logging/featureserviceschema.go

feast-ci-bot · 2022-04-27T18:00:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevjumba, pyalex

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kevjumba]

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

Signed-off-by: pyalex <[email protected]>

kevjumba · 2022-04-27T19:41:50Z

/lgtm

feast-ci-bot added the size/XXL label Apr 26, 2022

pyalex added the ok-to-test label Apr 26, 2022

pyalex requested a review from kevjumba April 26, 2022 23:44

kevjumba reviewed Apr 26, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

kevjumba reviewed Apr 26, 2022

View reviewed changes

pyalex force-pushed the decouple-go-logging branch from 4ec8ccd to f15b622 Compare April 27, 2022 00:04

kevjumba reviewed Apr 27, 2022

View reviewed changes

achals reviewed Apr 27, 2022

View reviewed changes

kevjumba approved these changes Apr 27, 2022

View reviewed changes

feast-ci-bot added the approved label Apr 27, 2022

decouple logging service

c7446f5

Signed-off-by: pyalex <[email protected]>

pyalex force-pushed the decouple-go-logging branch from 6f52c8f to c7446f5 Compare April 27, 2022 18:34

fix repo path in tests

dd563fb

Signed-off-by: pyalex <[email protected]>

feast-ci-bot assigned kevjumba Apr 27, 2022

feast-ci-bot added the lgtm label Apr 27, 2022

feast-ci-bot merged commit 5200fde into feast-dev:master Apr 27, 2022

pyalex mentioned this pull request May 2, 2022

feat: Write logged features to Offline Store (Go - Python integration) #2621

Merged

Comments

Conversation

pyalex commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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!

pyalex Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pyalex Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feast-ci-bot commented Apr 27, 2022

Uh oh!

kevjumba commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pyalex commented Apr 26, 2022 •

edited

Loading

codecov-commenter commented Apr 26, 2022 •

edited

Loading

pyalex Apr 27, 2022 •

edited

Loading

pyalex Apr 27, 2022 •

edited

Loading