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

[Containerapp] az containerapp env telemetry: add command for updating telemetry settings #7199

Merged
merged 45 commits into from
Mar 5, 2024

Conversation

michaelkira
Copy link
Contributor

@michaelkira michaelkira commented Jan 19, 2024


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

Copy link

azure-client-tools-bot-prd bot commented Jan 19, 2024

⚠️Azure CLI Extensions Breaking Change Test
⚠️containerapp
rule cmd_name rule_message suggest_message
⚠️ 1011 - SubgroupAdd containerapp env telemetry sub group containerapp env telemetry added

Copy link

Hi @michaelkira,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Jan 19, 2024

Thank you for your contribution! We will review the pull request and get back to you soon.

@michaelkira michaelkira marked this pull request as ready for review January 24, 2024 06:48
@michaelkira michaelkira changed the title Add otel commands Add containerapp env telemetry preview Jan 24, 2024
@yonzhan
Copy link
Collaborator

yonzhan commented Jan 24, 2024

Please fix CI issues.

@michaelkira michaelkira changed the title Add containerapp env telemetry preview [Containerapp] az containerapp env telemetry: add command for telemetry settings Jan 24, 2024
@michaelkira michaelkira changed the title [Containerapp] az containerapp env telemetry: add command for telemetry settings [Containerapp] az containerapp env telemetry: add command for updating telemetry settings Jan 24, 2024
Copy link
Contributor

@Greedygre Greedygre left a comment

Choose a reason for hiding this comment

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

Please resolve the conflict and add two blank lines between functions.


# Container Apps Telemetry Commands

helps['containerapp env telemetry data-dog set'] = """
Copy link
Contributor

Choose a reason for hiding this comment

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

add help information for 'containerapp env telemetry'


with self.command_group('containerapp env telemetry data-dog', is_preview=True) as g:
g.custom_command('set', 'set_environment_telemetry_data_dog', supports_no_wait=True, exception_handler=ex_handler_factory())
g.custom_command('delete', 'delete_environment_telemetry_data_dog', supports_no_wait=True, exception_handler=ex_handler_factory())
Copy link
Contributor

Choose a reason for hiding this comment

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

confirmation

return self.get_param("enable_open_telemetry_metrics")

def construct_payload(self):
# Get containerapp env properties of CA we are updating
Copy link
Contributor

Choose a reason for hiding this comment

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

what does CA mean?

safe_set(self.managed_env_def, "properties", "openTelemetryConfiguration", "tracesConfiguration", "destinations", value=existing_traces)
elif not self.get_argument_enable_open_telemetry_traces() and DATA_DOG_DEST in existing_traces:
existing_traces.remove(DATA_DOG_DEST)
safe_set(self.managed_env_def, "properties", "openTelemetryConfiguration", "tracesConfiguration", "destinations", value=existing_traces)
Copy link
Contributor

@Juliehzl Juliehzl Mar 1, 2024

Choose a reason for hiding this comment

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

duplicated code with L63 and L67. could we set it outside if

Comment on lines +10 to +13
* 'az containerapp env telemetry data-dog set': support update environment data dog configuration with --site, --key, --enable-open-telemetry-traces and --enable-open-telemetry-metrics
* 'az containerapp env telemetry data-dog delete': support delete environment data dog configuration
* 'az containerapp env telemetry app-insights set': support update environment app insights configuration with --connection-string, --enable-open-telemetry-traces and --enable-open-telemetry-logs
* 'az containerapp env telemetry app-insights delete': support delete environment app insights configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move to the history.rst

time.sleep(5)
containerapp_env = self.cmd('containerapp env show -g {} -n {}'.format(resource_group, env_name)).get_output_in_json()

self.cmd('containerapp env show -n {} -g {}'.format(env_name, resource_group), checks=[
Copy link
Contributor

Choose a reason for hiding this comment

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

recommend to delete env in the end of test

@yanzhudd yanzhudd merged commit 8ea2cc2 into Azure:main Mar 5, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants