-
Notifications
You must be signed in to change notification settings - Fork 1.2k
change: Allow telemetry only in supported regions #5009
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
Changes from 1 commit
25f16ef
0ed85d6
b69ffcb
8d7f4a8
9321367
f972222
dadbb22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
from sagemaker.telemetry.constants import ( | ||
Feature, | ||
Status, | ||
Region, | ||
DEFAULT_AWS_REGION, | ||
) | ||
from sagemaker.user_agent import SDK_VERSION, process_studio_metadata_file | ||
|
@@ -189,8 +190,19 @@ def _send_telemetry_request( | |
"""Make GET request to an empty object in S3 bucket""" | ||
try: | ||
accountId = _get_accountId(session) if session else "NotAvailable" | ||
# telemetry will be sent to us-west-2 if no session availale | ||
region = _get_region_or_default(session) if session else DEFAULT_AWS_REGION | ||
|
||
# Validate region if session exists | ||
if session: | ||
region = _get_region_or_default(session) | ||
try: | ||
Region(region) | ||
except ValueError: | ||
logger.debug( | ||
"Region not found in supported regions. Telemetry request will not be emitted." | ||
) | ||
return | ||
else: # telemetry will be sent to us-west-2 if no session available | ||
region = DEFAULT_AWS_REGION | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isnt this redundant? This condition is already covered in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like we are assigning the same thing in 2 different places which is something we can simplify. By removing the if session condition here in line195 and handling everything within If session is None , it will automatically raise an Exception and will get assigned the default value in L#278. |
||
url = _construct_url( | ||
accountId, | ||
region, | ||
|
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.
Lets change to logger.warning