Skip to content

Conversation

pmajali
Copy link
Contributor

@pmajali pmajali commented Aug 20, 2025

📝 Description

  1. Adding Monitor in the regions endpoint. This specifies the monitor services enabled in a particular region.
  2. Adding group_by and filters field to the dashboard widget

✔️ How to Test

  1. Testing the regions endpoint
import os
import logging
from linode_api4 import LinodeClient

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

regions=client.regions()
for reg in regions:
    print(f"Region: {reg.id}")
    print(f"  Monitors - Alerts: {reg.monitors.alerts}")
    print(f"  Monitors - Metrics: {reg.monitors.metrics}")
    print("---")
  1. Test group_by and filter in dashboard widget
dashboard = client.monitor.dashboards(service_type="linode")
for d in dashboard:
    print("ID:", d.id)
    print("Service Type:", d.service_type)
    for widget in d.widgets:
        print("Label:", widget.label)
        print("Metric:", widget.metric)
        print("Unit:", widget.unit)
        print("group_by:", widget.group_by)
        if widget.filters:
            for f in widget.filters:
                print("label:", f.dimension_label)
                print("operator:", f.operator)
                print("values:", f.value)

How do I run the relevant unit/integration tests?

unit-test: make test-unit TEST_SUITE=monitor
integration tests: make testint TEST_SUITE=monitor

@pmajali pmajali requested a review from a team as a code owner August 20, 2025 09:30
@pmajali pmajali requested review from ezilber-akamai and yec-akamai and removed request for a team August 20, 2025 09:30
Copy link
Contributor

@Copilot 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 monitoring capabilities to the regions endpoint and enhances dashboard widgets with filtering and grouping functionality. It introduces monitor services information for regions and extends dashboard widget properties to support more advanced query capabilities.

  • Adds monitors field to regions showing which monitoring services are available (alerts and metrics)
  • Adds group_by and filters fields to dashboard widgets for more granular data querying
  • Includes alert configuration properties for monitor services

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

File Description
linode_api4/objects/region.py Adds RegionMonitors class and monitors property to Region class
linode_api4/objects/monitor.py Adds Filter and ServiceAlert classes, updates DashboardWidget with group_by and filters
test/fixtures/*.json Updates test fixtures with new monitor and widget properties
test/unit/objects/*_test.py Adds test coverage for new monitor and region functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"service_type": Property(ServiceType),
"type": Property(DashboardType),
"widgets": Property(List[DashboardWidget]),
"widgets": Property(json_object=DashboardWidget),
Copy link

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

The widgets property should be a list of DashboardWidget objects, but the current implementation will only handle a single widget. This should be Property(List[DashboardWidget]) to maintain the original list functionality.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the codebase handles all lists in this way, its specified like this and client can handle widget as list.

@yec-akamai
Copy link
Contributor

Overall looks good, can you fix the build issue please?

@pmajali
Copy link
Contributor Author

pmajali commented Sep 23, 2025

Overall looks good, can you fix the build issue please?

I'm already using the default_factory for filters field in dashboard, I still see this in build logs.
ValueError: mutable default <class 'types.SimpleNamespace'> for field filters is not allowed: use default_factory

Can you suggest how can this be resolved?

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 lint issue reported in the workflow? You can run make lint to check it locally

@yec-akamai yec-akamai requested a review from Copilot October 9, 2025 16:26
Copy link
Contributor

@Copilot 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 8 out of 8 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"service_type": Property(ServiceType),
"type": Property(DashboardType),
"widgets": Property(List[DashboardWidget]),
"widgets": Property(json_object=DashboardWidget),
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

The widgets property should use a list type since widgets is expected to be a list of DashboardWidget objects. This should be Property(List[DashboardWidget]) or Property(json_object=DashboardWidget, is_list=True) depending on the Property class implementation.

Suggested change
"widgets": Property(json_object=DashboardWidget),
"widgets": Property(json_object=DashboardWidget, is_list=True),

Copilot uses AI. Check for mistakes.

aggregate_function: AggregateFunction = ""
group_by: Optional[List[str]] = None
filters: Optional[List[Filter]] = None

Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

The filters field should specify the Filter type properly in the DashboardWidget class. Consider adding a Property mapping in the api_spec similar to how other nested objects are handled, to ensure proper deserialization of Filter objects.

Suggested change
properties = {
"filters": Property(json_object=Filter),
}

Copilot uses AI. Check for mistakes.

@yec-akamai yec-akamai requested a review from Copilot October 14, 2025 13:46
Copy link
Contributor

@Copilot 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 8 out of 8 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +131 to +144
def __getattribute__(self, name):
"""Override to handle the filters attribute specifically to avoid metaclass conflict."""
if name == "filters":
return object.__getattribute__(self, "_filters")
return object.__getattribute__(self, name)

def __setattr__(self, name, value):
"""Override to handle setting the filters attribute."""
if name == "filters":
object.__setattr__(self, "_filters", value)
else:
object.__setattr__(self, name, value)


Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

The custom __getattribute__ and __setattr__ methods add unnecessary complexity. Consider using a property decorator instead to handle the filters attribute mapping, which would be cleaner and more maintainable.

Suggested change
def __getattribute__(self, name):
"""Override to handle the filters attribute specifically to avoid metaclass conflict."""
if name == "filters":
return object.__getattribute__(self, "_filters")
return object.__getattribute__(self, name)
def __setattr__(self, name, value):
"""Override to handle setting the filters attribute."""
if name == "filters":
object.__setattr__(self, "_filters", value)
else:
object.__setattr__(self, name, value)
@property
def filters(self) -> Optional[List[Filter]]:
"""Property to access the filters attribute mapped to _filters."""
return self._filters
@filters.setter
def filters(self, value: Optional[List[Filter]]):
self._filters = value

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The JSON Metaclass creates a class attribute called filters that Python finds before our property in the attribute resolution order hence this approach does not work

"service_type": Property(ServiceType),
"type": Property(DashboardType),
"widgets": Property(List[DashboardWidget]),
"widgets": Property(json_object=DashboardWidget),
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

The widgets property should handle a list of DashboardWidget objects, but the current implementation only handles a single object. This should be Property(List[DashboardWidget]) or use appropriate list handling for json_object.

Suggested change
"widgets": Property(json_object=DashboardWidget),
"widgets": Property(json_object=DashboardWidget, is_list=True),

Copilot uses AI. Check for mistakes.

@yec-akamai
Copy link
Contributor

Can you add integration test case for group_by and filter? just want to make sure the field and type works properly. Otherwise looks good!

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.

2 participants