-
Notifications
You must be signed in to change notification settings - Fork 96
Support ACLP fetching entity metrics #777
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
Conversation
// FetchEntityMetrics returns metrics information for the individual entities within a specific service type | ||
func (mc *MonitorClient) FetchEntityMetrics(ctx context.Context, serviceType string, opts *EntityMetricsFetchOptions) (*EntityMetrics, error) { |
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.
// FetchEntityMetrics returns metrics information for the individual entities within a specific service type | |
func (mc *MonitorClient) FetchEntityMetrics(ctx context.Context, serviceType string, opts *EntityMetricsFetchOptions) (*EntityMetrics, error) { | |
// GetEntityMetrics returns metrics information for the individual entities within a specific service type | |
func (mc *MonitorClient) GetEntityMetrics(ctx context.Context, serviceType ServiceType, opts *GetEntityMetricsOptions) (*EntityMetrics, error) { |
Should we change Fetch
to Get
to be consistent with the API doc?
And I think the type should be the alias of string ServiceType
so users can use linodego.ServiceTypeDBaaS
without casting it into string (string(linodego.ServiceTypeDBaaS)
), and all other monitor functions may need this change as well.
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.
Using fetch
here is because this endpoint is actually a POST
. However, the prefix Get
usually indicates an actual GET
endpoint, which could be confusing. I've talked to the ACLP team about this situation, and they suggested fetch
might be a better way to go.
I'm not sure if using a ServiceType
string is better here. Because the ServiceType
enum includes a list of services, however, here we only accept dbaas for now. It could be misleading that we also support other service types.
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.
Okay, Fetch
seems to be fine then. Maybe there can be a chance to update the doc though. But ServiceType
is defined for this usage, right? Meaning we will get those services supported in the future?
Maybe checking with ACLP team for their opinion?
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 ServiceType
was defined for another set of endpoints, Dashboard. It's a separate effort though. From what I've learnt there isn't a clear plan for what services will be support in the next for Metrics here.
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.
Looks good and works well!
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.
Looks good on my end and is working locally.
📝 Description
This PR introduced a new client MonitorClient, to make calls to a different host monitor-api. It utilizes PAT token generated by linode api. The endpoint for this client is fetching metrics for a list of entities. Also, updated test suite for the new client and test the fetching metrics endpoint.
✔️ How to Test
Unit test:
Integration test: