-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add initial monitoring SLO resource #3363
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 33 files changed, 1242 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1214 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1214 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1214 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1214 insertions(+), 2 deletions(-)) |
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.
Don't forget the sidebar :)
products/monitoring/api.yaml
Outdated
- calendar_period | ||
description: | | ||
A rolling time period, semantically "in the past <rollingPeriod>". | ||
Must be an integer multiple of 1 day no larger than 30 days. |
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'm not totally sure from reading this how a user would specify it. Is this an integer, or a string?
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 is actually very dumb and must be X days in SECONDS (e.g. "rolling_period = 86400s") - I was wondering whether we wanted to change this to be an number and encode/decode this value (there are a couple of them)
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.
This is a really weird field and something i wanted to get feedback on (same with basicSli.latency.threshold) - based on my attempts to set this value, I believe the monitoring API only allows for "Xs", which means this needs to be like "$int days" in seconds, e.g. "86400s". Should we make this an int field for number of days and convert to the API-expected format?
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'd worry a bit about whether they eventually remove that restriction, though if we rename the field to "rolling_period_days" then we could go back and add a more generic "rolling_period" later. Maybe bring it up with the stackdriver folks to see if they have additional insight? What does gcloud do?
I think that if it ends up eventually supporting other types of duration strings then we'd want to leave it as-is, but if it's going to be seconds or days forever then an int would make more sense.
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.
Talked to them, and they are ok with having it as days (they originally wanted to have it as an int for days) - i'll add a rolling_period_days field
these resources are actually not integrated into gcloud yet
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! quick nit: maybe remove the part about "must be an integer multiple of 1 day" since that's built in now.
-%> | ||
// Name/Service Level Objective ID is a query parameter and cannot | ||
// be given in data | ||
delete(obj, "sloId") |
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'm a little confused here. It seems like the user can specify the sloId but it is being sent as name
. Why does this need to be explicitly deleted from the obj?
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.
yeah, i could do this better but I wasn't sure the best way. sloId (serviceLevelObjectiveId) is an optional query parameter on create that will get generated by the API, so we need to compute the value if it isn't given. url_param_only/input don't get read from the API response. I could remove this delete line since I don't think the API actually rejects it if given in data.
Edit: Nope, I forgot that I did this because it doesn't recognize the field and throws an error.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1224 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1224 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1224 insertions(+), 2 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1267 insertions(+), 2 deletions(-)) |
products/monitoring/api.yaml
Outdated
- calendar_period | ||
description: | | ||
A rolling time period, semantically "in the past <rollingPeriod>". | ||
Must be an integer multiple of 1 day no larger than 30 days. |
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! quick nit: maybe remove the part about "must be an integer multiple of 1 day" since that's built in now.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1267 insertions(+), 2 deletions(-)) |
* initial SLO resource * a space * indent * whitespace * add comment * sidebar * rename test fun * add validation, required * fmt * fix format code * change rolling period to int days * doc
Release Note Template for Downstream PRs (will be copied)