-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[metricbeat] [meraki] Ignore 400 error for unsupported per-device licensing #42397
base: main
Are you sure you want to change the base?
Conversation
|
getDeviceLicenses
funcThere 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!
@@ -150,6 +150,10 @@ func getDeviceChannelUtilization(client *meraki.Client, devices map[Serial]*Devi | |||
func getDeviceLicenses(client *meraki.Client, organizationID string, devices map[Serial]*Device) error { | |||
val, res, err := client.Organizations.GetOrganizationLicenses(organizationID, &meraki.GetOrganizationLicensesQueryParams{}) | |||
if err != nil { | |||
// Ignore 400 error for per-device licensing not supported | |||
if res.StatusCode() == 400 { |
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.
Shouldn't we check for any 4xx error and not just 400?
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.
Not sure if we should skip all 4xx error codes, 404 for example could mean the organization was not found meaning the org ID provided in the config is wrong.
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.
right. but it also can be something different. I mean I'm pretty sure that even 400 can be returned in multiple cases (something else can be invalid, might not be related to licensing not being supported, right?)
Overview
We should continue collecting metrics even if the
getDeviceLicenses
function errors with 400 status code.Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
Author's Checklist
How to test this PR locally
Related issues
getDeviceLicenses
fails for organizations that do not support per-device licensing #42395Use cases
Screenshots
Logs