ESD-889: Add telemetry to Port, MCR, MVE, VXC, and IX services - #150
ESD-889: Add telemetry to Port, MCR, MVE, VXC, and IX services#150Phil-Browne wants to merge 20 commits into
Conversation
Phil-Browne
changed the title
Phil-Browne
changed the title
There was a problem hiding this comment.
Pull request overview
Adds first-class SDK support for retrieving telemetry metrics for additional Megaport product types (VXC, Port, MCR, MVE, IX) via the /v2/product/{type}/{uid}/telemetry endpoints, reusing the shared ServiceTelemetryResponse model.
Changes:
- Introduces
Get*Telemetryrequest types + validation errors for VXC/Port/MCR/MVE/IX. - Adds
Get*Telemetrymethods to each corresponding service interface + implementation to call the telemetry endpoints with shared query parameters (type,daysorfrom/to). - Adds unit tests covering happy-path responses and parameter validation for each service.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| vxc_types.go | Adds GetVXCTelemetryRequest and VXC telemetry validation errors. |
| vxc.go | Adds GetVXCTelemetry to VXCService, plus request validation and endpoint call. |
| vxc_test.go | Adds tests for VXC telemetry retrieval and validation. |
| port_types.go | Adds GetPortTelemetryRequest and Port telemetry validation errors. |
| port.go | Adds GetPortTelemetry to PortService, plus request validation and endpoint call. |
| port_test.go | Adds tests for Port telemetry retrieval, from/to variant, and validation. |
| mve_types.go | Adds GetMVETelemetryRequest and MVE telemetry validation errors. |
| mve.go | Adds GetMVETelemetry to MVEService, plus request validation and endpoint call. |
| mve_test.go | Adds tests for MVE telemetry retrieval, from/to variant, and validation. |
| mcr_types.go | Adds GetMCRTelemetryRequest and MCR telemetry validation errors. |
| mcr.go | Adds GetMCRTelemetry to MCRService, plus request validation and endpoint call. |
| mcr_test.go | Adds tests for MCR telemetry retrieval, from/to variant, and validation. |
| ix_types.go | Adds GetIXTelemetryRequest and IX telemetry validation errors. |
| ix.go | Adds GetIXTelemetry to IXService, plus request validation and endpoint call. |
| ix_test.go | Adds tests for IX telemetry retrieval, from/to variant, and validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…er-bound tests; add IX peerUid decode test
- Add nil-request guard to all five validate functions (VXC, Port, MCR, MVE, IX) with new Err*TelemetryRequestRequired sentinel errors; avoids nil-pointer dereference when caller passes nil - Use PRODUCT_* constants in telemetry URL paths instead of hardcoded strings to prevent drift if path segments change - Add To-without-From validation test to all five service test suites - Add nil-request validation test to all five service test suites - Add TestGetVXCTelemetryFromTo happy-path test for the from/to time range query parameters
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ServiceTelemetryResponse, TelemetryTimeFrame, TelemetryMetricData, TelemetrySample, TelemetryUnit, and the TelemetrySample.UnmarshalJSON method were defined in nat_gateway_types.go but are used by Port, MCR, MVE, VXC, and IX services. Move them to shared_types.go so their cross-service scope is clear and they are easy to discover.
|
Addressing Copilot review feedback Breaking interface changes (intentional): Adding Shared telemetry types moved to All unit tests pass after the refactor. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed Copilot feedback: added
if err != nil {
if resp != nil && resp.Body != nil {
resp.Body.Close()
}
return nil, err
}
defer resp.Body.Close()Commit: 6ddc966. All unit tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Acknowledging Copilot's flags on These additions are intentional. The SDK is pre-v1, and adding methods to interfaces is an accepted practice at this stage — consistent with the same decision made for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 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.
…ce-telemetry # Conflicts: # mve_test.go
…ce-telemetry # Conflicts: # ix.go # ix_test.go # mcr_test.go # port_test.go # vxc_test.go
Replace six near-identical validate+fetch implementations with a shared validateTelemetryRequest/fetchTelemetry pair, keeping each service's public sentinel errors. Also validate that from precedes to and the range stays within the API's 180-day limit, name the service in sentinel messages, complete the documented metric types for Port and IX, drop a stray omitempty on the response-only PeerUID field, and replace deprecated reflect.Ptr with reflect.Pointer.
Phil-Browne
changed the title
Every telemetry test mocked a 2xx response, so the non-2xx and malformed-JSON paths in the shared fetchTelemetry helper were untested. Add one API-error test per service plus one malformed-JSON test covering the shared decode path. The per-service "valid values" doc comments read as if the SDK enforces membership; it only checks Types is non-empty. Note that the list isn't validated client-side rather than adding validation that would restate the upstream contract.
A null value in a [timestamp, value] tuple failed the decode of the whole series, so one gap in a metric emptied the response. The API emits null when a metric has no reading at a timestamp, and its own aggregation reads that as zero, so the decode now matches. Adds sample decode tests, including a null in the middle of a series, and aligns the VXC Types doc comment with its siblings.
Adds
GetVXCTelemetry,GetPortTelemetry,GetMCRTelemetry,GetMVETelemetry, andGetIXTelemetryto their respective services, backed by the/v2/product/{type}/{uid}/telemetryendpoints. All methods share the same parameter shape (metric types, optional time range or day count) and reuse the existingServiceTelemetryResponsetype. Validation and fetching are shared with the existing NAT Gateway telemetry via a common helper.Valid metric types per service (documented, not enforced client-side; the API rejects an invalid type):
A null value in a
[timestamp, value]sample tuple decodes as zero rather than failing the whole series. The API emits null when a metric has no reading at a timestamp, and its own aggregation reads null as zero, so one gap no longer empties a response.Sharing the validation helper also tightens NAT Gateway telemetry: it now rejects a
fromaftertoand a range over 180 days, neither of which the old NAT-specific validator checked. Existing NAT Gateway behavior is otherwise unchanged.Part of ESD-889.