Skip to content

adding SDK changes for ACLP APIs #528

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

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

Conversation

pmajali
Copy link

@pmajali pmajali commented Apr 11, 2025

📝 Description

The PR adds Monitor API for ACLP Product

✔️ How to Test

import os
import logging
from linode_api4 import LinodeClient

logger = logging.getLogger(__name__)

client = LinodeClient(os.environ.get("MY_PERSONAL_ACCESS_TOKEN"))
client.base_url = "https://api.linode.com/v4beta"


print("Listing services supported by ACLP")
services = client.monitor.supported_services()
for service in services:
        print("Label:", service.label)

How do I run the relevant unit/integration tests?

Run unit test as follows
pytest test/unit

Run integration test as follows
make testint TEST_SUITE=monitor

@pmajali pmajali requested a review from a team as a code owner April 11, 2025 06:20
@pmajali pmajali requested review from jriddle-linode and lgarber-akamai and removed request for a team April 11, 2025 06:20
@lgarber-akamai lgarber-akamai added the new-feature for new features in the changelog. label Apr 11, 2025
@yec-akamai
Copy link
Contributor

@pmajali Thank you for starting contributing to python SDK! Do you mind filling the PR description and test steps?

@@ -21,3 +21,4 @@
from .vpc import *
from .beta import *
from .placement import *
from .monitor import *

Check notice

Code scanning / CodeQL

'import *' may pollute namespace Note

Import pollutes the enclosing namespace, as the imported module
linode_api4.objects.monitor
does not define '__all__'.
Copy link
Author

Choose a reason for hiding this comment

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

updated the group and object to include 'all'

@yec-akamai yec-akamai self-requested a review April 18, 2025 15:27
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.

Can you fix the PR given the suggestion from code scanning?

@@ -10,6 +10,7 @@
from .lke import *
from .lke_tier import *
from .longview import *
from .monitor import *

Check notice

Code scanning / CodeQL

'import *' may pollute namespace Note

Import pollutes the enclosing namespace, as the imported module
linode_api4.groups.monitor
does not define '__all__'.
Copy link
Contributor

Choose a reason for hiding this comment

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

To address this issue, I'd suggest to change the namespace to a less general one, i.e., monitor_service.

@@ -10,6 +10,7 @@
from .lke import *
from .lke_tier import *
from .longview import *
from .monitor import *
Copy link
Contributor

Choose a reason for hiding this comment

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

To address this issue, I'd suggest to change the namespace to a less general one, i.e., monitor_service.

Comment on lines +21 to +34
class DashboardByService(Base):
"""
Get a dashboard: https://techdocs.akamai.com/linode-api/reference/get-dashboards
"""

properties = {
"id": Property(identifier=True),
"created": Property(is_datetime=True),
"label": Property(),
"service_type": Property(),
"type": Property(),
"widgets": Property(mutable=True),
"updated": Property(is_datetime=True),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This object seems has the exact same properties as Dashboard. It's not necessary to create a separate class here

"""

return self.client._get_and_filter(
DashboardByService,
Copy link
Contributor

Choose a reason for hiding this comment

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

The return type can be Dashboard

This group contains all features beneath the `/monitor` group in the API v4.
"""

def dashboards(self, *filters):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explicitly declare the return type please? Same for all functions in this PR

from linode_api4.objects import Base, Property


class Dashboard(Base):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a brief description to each class please?

Comment on lines +59 to +74
class MetricDefinition(Base):
"""
API Documentation: https://techdocs.akamai.com/linode-api/reference/get-monitor-information
"""

id_attribute = "metric"
properties = {
"available_aggregate_functions": Property(),
"dimensions": Property(mutable=True),
"label": Property(),
"is_alertable": Property(),
"metric": Property(),
"metric_type": Property(),
"scrape_interval": Property(),
"unit": Property(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems there is not an endpoint for getting a single metric. In this case, I think we can build it as an json object instead of class. Also personally i think it'd be better to name this type as ServiceMetric

Suggested change
class MetricDefinition(Base):
"""
API Documentation: https://techdocs.akamai.com/linode-api/reference/get-monitor-information
"""
id_attribute = "metric"
properties = {
"available_aggregate_functions": Property(),
"dimensions": Property(mutable=True),
"label": Property(),
"is_alertable": Property(),
"metric": Property(),
"metric_type": Property(),
"scrape_interval": Property(),
"unit": Property(),
}
class ServiceMetric(JSONObject):
# .. declare the attributes and their corresponding types

Comment on lines +77 to +82
class CreateToken(Base):
"""
API Documentation: https://techdocs.akamai.com/linode-api/reference/post-get-token
"""

properties = {"token": Property(mutable=True)}
Copy link
Contributor

Choose a reason for hiding this comment

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

This class is not necessary to have. You can build a MetricServiceToken as a JSONObject

endpoint=f"/monitor/services/{service_type}/metric-definitions",
)

def create_token(self, service_type: str, entity_ids: list, *filters):
Copy link
Contributor

Choose a reason for hiding this comment

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

filter is not needed since it's a post endpoint

Comment on lines +131 to +134
:param filters: Any number of filters to apply to this query.
See :doc:`Filtering Collections</linode_api4/objects/filtering>`
for more details on filtering.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you better describe the params needed?

Copy link
Contributor

@yec-akamai yec-akamai May 8, 2025

Choose a reason for hiding this comment

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

There are many properties that are enums in the objects. Can you build them as StrEnum, like we did in the linodego https://github.com/linode/linodego/pull/722/files?

Plus, it might be good to align the naming with lindoego too

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.

You'll have to sign all of you commits (make sure each commit has the green Verified tag) before merging this PR. To save troubles in the end, I'd suggest to do it sooner than later. You can look for github official docs to add signatures to all your commits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature for new features in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants