-
Notifications
You must be signed in to change notification settings - Fork 276
Track usage, errors, and create disable_telemetry flag #2319
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Some of the functions like _build_path, retrieve_id, etc. can be marked as property.
| def item_count(self) -> int: | ||
| """Get the number of items in the current payload.""" | ||
| return len(self.items) | ||
|
|
||
| def is_empty(self) -> bool: | ||
| """Check if the payload is empty.""" | ||
| return len(self.items) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark these with @Property
| def is_informational_logging_enabled(self) -> bool: | ||
| """Check if informational level logging is enabled.""" | ||
| return self.logger.isEnabledFor(logging.INFO) | ||
|
|
||
| def is_error_logging_enabled(self) -> bool: | ||
| """Check if error level logging is enabled.""" | ||
| return self.logger.isEnabledFor(logging.ERROR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark these with @Property
| ORT_SUPPORT_DIR = r"Microsoft/DeveloperTools/.onnxruntime" | ||
|
|
||
|
|
||
| def get_telemetry_base_dir() -> Path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it a property and also mark it with @functools.lru_cache(maxsize=1)
|
|
||
|
|
||
| def get_telemetry_base_dir() -> Path: | ||
| os_name = platform.system() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a way for user to override this path with a known/defined environment variable OLIVE_TELEMETRY_CACHE_PATH or something. Permissions might be a problem sometimes in default folders.
Add telemetry library and Olive-specific functionality
--disable-telemetry)Nits:
Description of change for release notes:
Checklist before requesting a review
lintrunner -a