-
Notifications
You must be signed in to change notification settings - Fork 97
ACLP Alerting go SDK #824
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
base: main
Are you sure you want to change the base?
ACLP Alerting go SDK #824
Conversation
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.
Pull Request Overview
This PR introduces ACLP (Alerting Linode Platform) SDK functionality to the linodego client library, adding support for monitor alert definitions and channels. The implementation includes API client methods, data structures, and comprehensive test coverage.
- Adds monitor alert definitions CRUD operations with support for v4beta API endpoints
- Implements monitor channels functionality for alert notification management
- Provides comprehensive unit and integration test coverage for all new functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| monitor_alert_definitions.go | Core implementation of alert definitions API client methods and data structures |
| monitor_channels.go | Monitor channels API client methods and channel management functionality |
| request_helpers.go | New helper function for v4beta API endpoint formatting |
| test/unit/base.go | Enhanced test base to support v4beta endpoint mocking |
| test/unit/monitor_alert_definitions_test.go | Comprehensive unit tests for alert definitions functionality |
| test/integration/monitor_alert_definitions_test.go | Integration test with real API interaction scenarios |
| test/integration/fixtures/TestMonitorAlertDefinition_instance.yaml | Test fixture data for integration testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
9226cfb to
8e71881
Compare
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.
Pull Request Overview
Copilot reviewed 77 out of 81 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yec-akamai
left a comment
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.
It seems some unrelated commits are added accidentally. Do you mind cleaning up the branch and fixing the conflicts?
|
@srbhaakamai Just bumping Ye's comment — could you resolve the conflicts and unrelated comments on this branch? I'm happy to do it too and push up the new commit if you'd like |
7818a6b to
aed99f8
Compare
|
@srbhaakamai I just finished up fixing up the commit history and will be providing a review shortly 👍 |
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.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
All the review comments are addressed, tested. please continue to review/approve this. Srinidhi |
ezilber-akamai
left a comment
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 failure in TestInstance_GetMonthlyTransfer should be fixable by simply running make fixtures TEST_ARGS="-run TestInstance_GetMonthlyTransfer" and pushing up the newly generated fixture. The failure in TestMonitorAlertDefinition_smoke however seems like a potential issue.
|
=== RUN TestMonitorAlertDefinition_smoke LINODE_TOKEN token configured in integration_test_suite.go init() may not have read/write access to monitor. e.g., token configured linode/linode_api4-python#589 in this seems to work. Please suggest or make change as needed as i dont have access to change the environment details. |
|
@ezilber-akamai |
|
@srbhaakamai I just checked and the token does have read/write access to monitor. I'm thinking it could be an issue with the fixtures. When I run against the fixture locally, I also get the 401 but when I run against the API it passes. I'm looking into this. |
ezilber-akamai
left a comment
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 for the contribution!
yec-akamai
left a comment
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.
Can you fix the comments as well as the merge conflicts? Thank you!
| toolchain go1.25.1 | ||
|
|
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.
Can you revert the change to go mod? should not be necessary here
| ServiceType string `json:"service_type"` | ||
| Status string `json:"status"` | ||
| HasMoreResources bool `json:"has_more_resources"` | ||
| Rule *Rule `json:"rule"` |
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.
I don't think this field rule is under this struct in the api doc. It should be removed from here.
| Updated *time.Time `json:"-"` | ||
| UpdatedBy string `json:"updated_by"` | ||
| CreatedBy string `json:"created_by"` | ||
| EntityIDs []string `json:"entity_ids"` |
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.
Judging from the api doc, the entity_ids seems could be any type: The format type for an entity_id may vary, based on the Akamai Cloud service_type
You should build them as []any to accommodate that.
| Threshold *float64 `json:"threshold,omitempty"` | ||
| Unit *string `json:"unit,omitempty"` |
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.
Just curious why is is necessary to use pointer for these two fields?
|
|
||
| // AlertDefinitionCreateOptions are the options used to create a new alert definition. | ||
| type AlertDefinitionCreateOptions struct { | ||
| ServiceType string `json:"service_type"` // mandatory |
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.
You don't need to include this field here since it's a path param.
| if serviceType != "" { | ||
| endpoint = formatAPIPath("monitor/services/%s/alert-definitions", serviceType) | ||
| } else { | ||
| endpoint = formatAPIPath("monitor/alert-definitions") | ||
| } |
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.
You probably need to separate these two endpoints into two functions in linodego. Because go can't take optional parameters.
| // AlertChannelCreateOptions are the options used to create a new Monitor Channel. | ||
| type AlertChannelCreateOptions struct { | ||
| Label string `json:"label"` | ||
| Type string `json:"type"` | ||
| Details AlertChannelDetailOptions `json:"details"` | ||
| } | ||
|
|
||
| // AlertChannelDetailOptions are the options used to create the details of a new Monitor Channel. | ||
| type AlertChannelDetailOptions struct { | ||
| To string `json:"to,omitempty"` | ||
| } |
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.
It seems only GET endpoints are supported currently. Can you remove the CreateOptions?
| func (c *Client) GetAlertChannel(ctx context.Context, channelID int) (*AlertChannel, error) { | ||
| e := formatAPIPath("monitor/alert-channels/%d", channelID) | ||
| return doGETRequest[AlertChannel](ctx, c, e) | ||
| } |
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.
Just curious, is getting a single alert channel available now? Didn't see it in the api doc though
| }, | ||
| } | ||
|
|
||
| createdAlert, err := client.CreateMonitorAlertDefinition(context.Background(), testMonitorAlertDefinitionServiceType, createOpts) |
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.
Can you add a test to the CreateMonitorAlertDefinitionWithIdempotency as well? Just want to make sure the API takes the request as expected
| @@ -0,0 +1,189 @@ | |||
| package integration | |||
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.
Can you add test cases to the list endpoints ListMonitorAlertDefinitions and ListMonitorAlertChannels?
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.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Rule represents a single rule for an alert. | ||
| type Rule struct { |
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 Rule object is not the same as it is in the create options. i.e. unit and label is not part of the create body param. Can you double check it, and create a separate option struct RuleOptions to distinguish them? There could be other fields which diverge on read and write path that I may miss, can you double check them as well?
| // DimensionFilter represents a single dimension filter used inside a Rule. | ||
| type DimensionFilter struct { | ||
| DimensionLabel string `json:"dimension_label"` | ||
| Label string `json:"label"` |
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.
I find label should not be part of the create options too. It only in the return result
📝 Description
Provide go SDK support to ACLP clients to perform CRUD operations for Alerts.
✔️ How to Test
How do I run the relevant unit/integration tests?
Prerequisites
Go 1.19+ installed
Valid Linode API token with monitor permissions
Export LINODE_TOKEN environment variable for integration tests
Run all monitor alert definition unit tests:
UNIT TEST:
go test ./test/unit -run ".MonitorAlertDefinition." -v
Expected Output:
✅ TestCreateMonitorAlertDefinition
✅ TestCreateMonitorAlertDefinitionWithIdempotency
✅ TestGetMonitorAlertDefinition
✅ TestListMonitorAlertDefinitions
✅ TestUpdateMonitorAlertDefinition
✅ TestUpdateMonitorAlertDefinition_LabelOnly
✅ TestDeleteMonitorAlertDefinition
INTEGRATION TEST:
export LINODE_TOKEN="your-linode-api-token"
go test ./test/integration -run "TestMonitorAlertDefinition_smoke" -v
Test Coverage:
List Operations: Validates fetching existing alert definitions
Channel Discovery: Finds available alert channels for testing
Create: Tests complex alert definition creation with:
TriggerConditions (evaluation periods, polling intervals)
RuleCriteria with Rules (metrics, operators, thresholds)
DimensionFilters (node type filtering)
Update: Tests label-only updates with proper timing
Delete: Tests cleanup with exponential backoff retry logic
Expected Duration: ~102 seconds (includes API timing requirements)
📷 Preview
If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.