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

Add opt-out exception logging telemetry #1535

Merged
merged 5 commits into from
May 19, 2021

Conversation

jklegar
Copy link
Collaborator

@jklegar jklegar commented May 5, 2021

What this PR does / why we need it: Telemetry will now log anonymized exceptions from Feast methods in addition to usage statistics

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Telemetry will now log anonymized exceptions from Feast methods in addition to usage statistics. To opt out, see https://docs.feast.dev/v/master/feast-on-kubernetes/advanced-1/telemetry

@jklegar
Copy link
Collaborator Author

jklegar commented May 5, 2021

/kind housekeeping

jklegar added 4 commits May 17, 2021 14:17
Signed-off-by: Jacob Klegar <[email protected]>
Signed-off-by: Jacob Klegar <[email protected]>
Signed-off-by: Jacob Klegar <[email protected]>
Signed-off-by: Jacob Klegar <[email protected]>
@jklegar jklegar force-pushed the jacob_exception_logging branch from 7ad1b33 to 8265940 Compare May 17, 2021 18:19
@@ -2,9 +2,9 @@

## How telemetry is used

The Feast maintainers use anonymous usage statistics to help shape the Feast roadmap. Several client methods are tracked, beginning in Feast 0.9. Users are assigned a UUID which is sent along with the name of the method, the Feast version, the OS \(using `sys.platform`\), and the current time. For more detailed information see [the source code](https://github.com/feast-dev/feast/blob/master/sdk/python/feast/telemetry.py).
The Feast maintainers use anonymous usage statistics and error tracking to help shape the Feast roadmap. Several client methods are tracked, beginning in Feast 0.9. Users are assigned a UUID which is sent along with the name of the method, the Feast version, the OS \(using `sys.platform`\), and the current time. For more detailed information see [the source code](https://github.com/feast-dev/feast/blob/master/sdk/python/feast/telemetry.py).
Copy link
Member

Choose a reason for hiding this comment

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

@jklegar can we pull the Telemetry docs out of feast-on-kubernetes? What about creating a Telemetry page under Reference?

f.write(self._telemetry_id)
print(
"Feast is an open source project that collects anonymized error reporting and usage statistics. To opt out or learn"
" more see https://docs.feast.dev/v/master/feast-on-kubernetes/advanced-1/telemetry"
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I think we should create a nicer URL. We also shouldnt point to master.

self._telemetry_enabled = False
self.check_env()

def check_env(self):
Copy link
Member

Choose a reason for hiding this comment

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

check is not an appropriate name if we are actually writing and initializing Telemetry.

Signed-off-by: Jacob Klegar <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2021

Codecov Report

Merging #1535 (3c7776d) into master (73ac3de) will decrease coverage by 0.04%.
The diff coverage is 76.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1535      +/-   ##
==========================================
- Coverage   83.72%   83.67%   -0.05%     
==========================================
  Files          65       65              
  Lines        5634     5703      +69     
==========================================
+ Hits         4717     4772      +55     
- Misses        917      931      +14     
Flag Coverage Δ
integrationtests 83.58% <76.84%> (-0.05%) ⬇️
unittests 78.69% <76.84%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
sdk/python/feast/cli.py 45.77% <ø> (+0.38%) ⬆️
sdk/python/feast/telemetry.py 62.62% <68.11%> (+14.89%) ⬆️
sdk/python/feast/entity.py 96.36% <100.00%> (+0.06%) ⬆️
sdk/python/feast/feature_store.py 91.89% <100.00%> (+0.07%) ⬆️
sdk/python/feast/feature_view.py 87.75% <100.00%> (+0.25%) ⬆️
sdk/python/feast/repo_config.py 97.29% <100.00%> (+0.07%) ⬆️
sdk/python/feast/repo_operations.py 32.08% <100.00%> (+1.86%) ⬆️

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 73ac3de...3c7776d. Read the comment docs.

@woop
Copy link
Member

woop commented May 19, 2021

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jklegar, woop

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:

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

@feast-ci-bot feast-ci-bot merged commit cb230ae into feast-dev:master May 19, 2021
woop pushed a commit that referenced this pull request May 19, 2021
* Add opt-out exception logging

Signed-off-by: Jacob Klegar <[email protected]>

* lint

Signed-off-by: Jacob Klegar <[email protected]>

* Update docs page

Signed-off-by: Jacob Klegar <[email protected]>

* Use global Telemetry object

Signed-off-by: Jacob Klegar <[email protected]>

* Address comments

Signed-off-by: Jacob Klegar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants