-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Sandraho/add app insights #40327
base: main
Are you sure you want to change the base?
Sandraho/add app insights #40327
Conversation
…class. might need to move to AIProjectClient class instead.
…e to check how service will link into sdk
Thank you for your contribution @sho1216! We will review the pull request and get back to you soon. |
@sho1216 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
@sho1216 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
@@ -797,7 +806,15 @@ def get_connection_string(self) -> str: | |||
) | |||
|
|||
if not get_workspace_response.properties.application_insights: |
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.
The same as the function above
@@ -616,7 +664,16 @@ async def get_connection_string(self) -> str: | |||
) | |||
|
|||
if not get_workspace_response.properties.application_insights: | |||
raise ResourceNotFoundError("Application Insights resource was not enabled for this Project.") | |||
# create an AppInsights for the customer using the resource group and project name |
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.
Please don't add "write" operation within a "read" function. Let's create another function get_or_create_connection_string
, and then we can discuss with Azure SDK team for whether we keep both 2 functions, or only get_or_create_connection_string
.
@@ -5401,6 +5401,46 @@ def __init__(self, *args, **kwargs) -> None: | |||
self._serialize: Serializer = input_args.pop(0) if input_args else kwargs.pop("serializer") | |||
self._deserialize: Deserializer = input_args.pop(0) if input_args else kwargs.pop("deserializer") | |||
|
|||
@distributed_trace_async | |||
async def create_application_insights_if_not_exists(self, resource_group_name: str, resource_name: str, location: str) -> str: |
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.
Thanks! I'll try the implementation of this function shortly. At first lance, this is the operations for async calls, while this function seems to be sync?
… want to opt in for app insights
@@ -25,6 +25,15 @@ | |||
EvaluationsOperations, | |||
TelemetryOperations, | |||
) | |||
from azure.identity import DefaultAzureCredential | |||
from azure.identity import DefaultAzureCredential |
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.
Double import?
@@ -33,6 +33,16 @@ | |||
from azure.core.credentials import TokenCredential | |||
from azure.core.exceptions import ResourceNotFoundError | |||
from azure.core.tracing.decorator_async import distributed_trace_async | |||
from azure.identity import DefaultAzureCredential |
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.
Why do you need to add all of these dependencies that aren't used in the code?
from azure.core.tracing.decorator import distributed_trace | ||
from azure.mgmt.applicationinsights import ApplicationInsightsManagementClient | ||
from azure.mgmt.applicationinsights.v2018_05_01_preview.models import ApplicationInsightsComponent | ||
from azure.identity import DefaultAzureCredential |
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.
Double import of this from above.
) | ||
if not connection_string: | ||
raise ValueError("Connection string cannot be None") | ||
credential = DefaultAzureCredential() |
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.
Why do you need to redefine the credential value here from the value above?
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines