Skip to content

Conversation

@vivkumar-akamai
Copy link

@vivkumar-akamai vivkumar-akamai commented Oct 24, 2025

📝 Description

This is linode CLI external plugin for ACLP-MR services. Here is how this requests going to looks like

Sample Command
linode-cli monitor-metrics-get --url 'https://mr-devcloud.cloud-observability-dev.akadns.net/v2beta/monitor/services/firewall/metrics'
--header 'Authorization: <redacted>'
--header 'Authentication-type: jwe'
--header 'Pragma: akamai-x-get-extracted-values'
--header 'Userid: 10001'
--header 'Content-Type: application/json'
--data '{"entity_ids":[1001], "relative_time_duration":{"unit":"min","value":60}, "metrics":[{"aggregate_function":"sum","name":"fw_ingress_bytes_accepted"}],"filters": [],
"associated_entity_region": "us-east", "time_granularity": {"value": 1, "unit": "days"}}'
['Authorization: Bearer <redacted>', 'Authentication-type: jwe', 'Pragma: akamai-x-get-extracted-values', 'Userid: 10001', 'Content-Type: application/json']
{'entity_ids': [1001], 'relative_time_duration': {'unit': 'min', 'value': 60}, 'metrics': [{'aggregate_function': 'sum', 'name': 'fw_ingress_bytes_accepted'}], 'filters': [], 'associated_entity_region': 'us-east', 'time_granularity': {'value': 1, 'unit': 'days'}}

Response
{
"data": {
"result": [],
"resultType": "matrix"
},
"isPartial": false,
"stats": {
"executionTimeMsec": 1,
"seriesFetched": "0"
},
"status": "success"
}

✔️ How to Test

What are the steps to reproduce the issue or verify the changes?

How do I run the relevant unit/integration tests?

📷 Preview

If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.

@vivkumar-akamai vivkumar-akamai requested a review from a team as a code owner October 24, 2025 11:17
@vivkumar-akamai vivkumar-akamai requested review from jriddle-linode and zliang-akamai and removed request for a team October 24, 2025 11:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds an external CLI plugin for the ACLP Metrics Read (ACLP-MR) service, enabling users to query monitoring metrics through the Linode CLI.

Key Changes:

  • New plugin monitor-metrics-get.py that makes HTTP POST requests to ACLP Metric Read Service endpoints
  • Supports custom headers, request payload, timeout configuration, and CA certificate validation
  • Implements argument parsing with mandatory fields validation for metrics queries

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"--cacert", "-c",
type=str,
help="Add ca certificate to validate server",
default=False
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

The default value for --cacert should be None or True, not False. Setting it to False will cause requests.post() to skip SSL certificate verification when no certificate is provided, which is a security risk. The verify parameter expects either a boolean or a path string.

Suggested change
default=False
default=None

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

Its server validation certs. I don't think it should be concern

Copy link
Contributor

Choose a reason for hiding this comment

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

@vivkumar-akamai Could you elaborate a bit on this? I'd assume we'd always want to validate the remote certificate unless the user explicitly specifies otherwise but I might just be missing something

@zliang-akamai
Copy link
Member

zliang-akamai commented Nov 20, 2025

@vivkumar-akamai Be aware that this is the public GitHub site, and if your token is still valid, please hide it from the PR description you post here. I just redacted them.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +65 to +66
def data_parser(args):
return args.data
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

[nitpick] The data_parser function is unnecessary as it only returns args.data without any transformation. Consider removing this function and accessing args.data directly on line 77.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@yec-akamai yec-akamai left a comment

Choose a reason for hiding this comment

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

Sample Command
linode-cli monitor-metrics-get --url 'https://mr-devcloud.cloud-observability-dev.akadns.net/v2beta/monitor/services/firewall/metrics'
--header 'Authorization: '
--header 'Authentication-type: jwe'
--header 'Pragma: akamai-x-get-extracted-values'
--header 'Userid: 10001'
--header 'Content-Type: application/json'
--data '{"entity_ids":[1001], "relative_time_duration":{"unit":"min","value":60}, "metrics":

The current implementation looks not ideal from a CLI user experience. It's more like a curl command.
I wonder if you can build a plugin as the existing plugins in our codebase, i.e. linode-cli metadata. You can look for docs and examples under /plugins: https://github.com/linode/linode-cli/tree/dev/linodecli/plugins

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.

5 participants