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

SNOW-644849: Add telemetry about imported pacakages at runtime #1236

Merged
merged 5 commits into from
Sep 1, 2022

Conversation

sfc-gh-jdu
Copy link
Collaborator

@sfc-gh-jdu sfc-gh-jdu commented Aug 24, 2022

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    SNOW-644849

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    We would like to know what packages are imported at Python runtime along with connector, which can help us understand the user scenarios of python connector. The message will looks like this:

{
  "source": "PythonSnowpark",
  "type": "client_imported_packages",
  "value": "{'pdb', 'bz2', 'abc', 'ssl', 'faulthandler', 'fnmatch', 'pytest', 'shlex', 'fractions', 'enum', 'queue', 'certifi', 'traceback', 'pprint', 'pwd', 'pickle', 'ast', 'uu', 'pkg_resources', 'codecs', 'gc', 'hmac', 'copy', 'errno', 'copyreg', 'subprocess', 'snowflake', 'weakref', 'decimal', 'logging', 'dataclasses', 'typing', 'numpy', 'pytest_forked', 'ipaddress', 'operator', 'signal', 'cmd', 'email', 'gettext', 'datetime', 'importlib', 'functools', 'base64', 'calendar', 'configparser', 'codeop', 'tempfile', 'cryptography', 'distutils', 'dis', 'contextvars', 'linecache', 'uuid', 'contextlib', 'posixpath', 'unittest', 'Cryptodome', 'selectors', 'iniconfig', 'binascii', 'locale', 'grp', 'select', 'encodings', 'posix', 'plistlib', 'sre_compile', 'platform', 'pycparser', 'ntpath', 'time', 'itertools', 'textwrap', 'csv', 'zlib', 'unicodedata', 'token', 'sys', 'struct', 'reprlib', 'keyword', 'charset_normalizer', 'sysconfig', 'cffi', 'attr', 'socket', 'pyexpat', 'numbers', 'jwt', 'teamcity', 'pathlib', 'random', 'opcode', 'py', 'json', 'threading', 'zipfile', 'ctypes', 'math', 'code', 'gzip', 'marshal', 'collections', 'genericpath', 'os', 'mimetypes', 'pytz', 'xml', 'concurrent', 'bdb', 'string', 'difflib', 'argparse', 'quopri', 'http', 'inspect', 'warnings', 'pluggy', 'hashlib', 'urllib', 'zipimport', 'OpenSSL', 'bisect', 'site', 'glob', 'stat', 'html', 'multiprocessing', 'atexit', 'io', 'secrets', 'sre_constants', 'lzma', 'heapq', 'tokenize', 'pkgutil', 'sre_parse', 'shutil', 'tomli', 'webbrowser', 'asyncio', 'asn1crypto', 'stringprep', 'test', 're', 'builtins', 'cython_runtime', 'types', 'array'}"
}

@sfc-gh-jdu sfc-gh-jdu requested a review from a team August 24, 2022 00:25
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022

Codecov Report

Merging #1236 (b7719d2) into main (2c3f6d5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
+ Coverage   81.63%   81.65%   +0.01%     
==========================================
  Files          59       59              
  Lines        8522     8531       +9     
  Branches     1379     1382       +3     
==========================================
+ Hits         6957     6966       +9     
- Misses       1258     1259       +1     
+ Partials      307      306       -1     
Impacted Files Coverage Δ
src/snowflake/connector/connection.py 90.78% <100.00%> (+0.27%) ⬆️
src/snowflake/connector/telemetry.py 92.75% <100.00%> (+0.05%) ⬆️
src/snowflake/connector/errors.py 89.78% <0.00%> (-0.54%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sfc-gh-sfan
Copy link
Contributor

sfc-gh-sfan commented Aug 24, 2022

I kinda agree this is a little bit too hacky...(scratched my comment about sp as I thought this is snowpark by mistake).

@sfc-gh-jdu
Copy link
Collaborator Author

I kinda agree this is a little bit too hacky...(scratched my comment about sp as I thought this is snowpark by mistake).

It will not work in SP (similar to snowflakedb/snowpark-python#274), and I don't expect it works in SP because per our discussion, we can just collect pakcages information of a SP from snowhouse. Though it's pretty hacky, it still works (you can find in the test) and I'm just looking for a better way to do that...

@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-644849-imported-packages-telemetry branch from 73aed53 to 84e7c3d Compare August 25, 2022 01:18
@sfc-gh-jdu sfc-gh-jdu marked this pull request as ready for review August 25, 2022 01:26
@sfc-gh-jdu
Copy link
Collaborator Author

sfc-gh-jdu commented Aug 25, 2022

I changed to use sys.modules that we can capture the usages of packages other than pandas. Note that as long as pandas exists in the environment, it will be pulled into sys.modules. But our goal is actually to get some information about what packages are used along with connector (or snowpark) and pandas. We are not able to get pandas (also pyarrow) usage in this telemetry records.

@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-644849-imported-packages-telemetry branch from 8599c63 to acca9f8 Compare August 26, 2022 18:01
Copy link
Contributor

@sfc-gh-kwagner sfc-gh-kwagner left a comment

Choose a reason for hiding this comment

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

LGTM

src/snowflake/connector/connection.py Show resolved Hide resolved
test/integ/test_connection.py Outdated Show resolved Hide resolved
src/snowflake/connector/__init__.py Outdated Show resolved Hide resolved
src/snowflake/connector/connection.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@sfc-gh-jdu sfc-gh-jdu merged commit 4ccba6a into main Sep 1, 2022
@sfc-gh-jdu sfc-gh-jdu deleted the jdu-SNOW-644849-imported-packages-telemetry branch September 1, 2022 00:23
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants