RFC 0673: ApplicationSignals - ServiceLevelObjective L2 #714
Conversation
moelasmar
changed the title
|
|
||
| ### CHANGELOG | ||
|
|
||
| `feat(cloudwatch): introduce new Application Signals L2 constructs for simplifying SLO creation and management` |
There was a problem hiding this comment.
the new module will be added to the package aws-applicationsignals , so I recommend change the title to be feat(applicationSignals)
| * ERIOD_SLI_METRIC | ||
| * REQUEST_SLI_METRIC |
There was a problem hiding this comment.
seems these lines are mistakenly added.
| |name |string |The name of the SLO | | ||
| |keyAttributes |IResolvable \| { [string]: string } |The key attributes for the SLO | | ||
| |operationName? |string |The operation name for the SLO (optional) | | ||
| |goal |SLOGoalConfig |The goal configuration for the SLO | |
There was a problem hiding this comment.
are SLOGoalConfig type, and GoalConfig Type (you mentioned in line 33) the same? If yes, so please use one of these names
| |name |string |The name of the SLO | | ||
| |keyAttributes |IResolvable \| { [string]: string } |The key attributes for the SLO | | ||
| |operationName? |string |The operation name for the SLO (optional) | | ||
| |goal |SLOGoalConfig |The goal configuration for the SLO | |
There was a problem hiding this comment.
same comment on PeriodBasedSloProps.goal
| #### IntervalProps | ||
|
|
||
| |Name |Type |Description | | ||
| |--- |--- |--- | | ||
| |duration |number |Duration value | | ||
| |unit |DurationUnit |Unit of duration <br>One of the following enum values:<br>`HOUR`<br>`DAY`<br>`MINUTE`| | ||
|
|
||
| #### CalendarIntervalProps | ||
|
|
||
| |Name |Type |Description | | ||
| |--- |--- |--- | | ||
| |duration |number |Duration value | | ||
| |startTime? |number |Start time for the calendar interval.<br> default is now | | ||
| |unit |DurationUnit (enum) |Unit of duration. the following enum values:<br>`MONTH` | |
There was a problem hiding this comment.
can you mention that both types implement IInterval, as below you refer to IInterval.
| |metricType? |MetricType(enum) |Optional metric type. <br>One of the following enum values:<br>`LATENCY`<br>`AVAILABILITY` | | ||
| |keyAttributes? |string |Optional key attributes | | ||
| |operationName? |string |Optional operation name | | ||
| |comparisonOperator? |ComparisonOperator(enum( |Optional comparison operator. <br> One of the following enum values:<br>`GREATER_THAN`<br>`LESS_THAN`<br>`GREATER_THAN_OR_EQUAL`<br>`LESS_THAN_OR_EQUAL` |
| #### PeriodBasedMetricProps | ||
|
|
||
| |Name |Type |Description | | ||
| |--- |--- |--- | | ||
| |periodSeconds? |number |Period in seconds, default is 60s | | ||
| |statistic |MetricStatistic |Statistic | | ||
| |metricDataQueries |MetricDataQuery[] |Array of metric queries | | ||
|
|
||
| #### RequestBasedMetricProps | ||
|
|
||
| |Name |Type |Description | | ||
| |--- |--- |--- | | ||
| |goodCountMetrics? |MetricDataQuery[] |Optional good count metrics | | ||
| |totalCountMetrics |MetricDataQuery[] |Total count metrics | | ||
| |badCountMetrics? |MetricDataQuery[] |Optional bad count metrics | |
There was a problem hiding this comment.
please mention that both of these types implements SliMetricBaseProps
| |Name |Type |Description | | ||
| |--- |--- |--- | | ||
| |name |string |The name of the SLO | | ||
| |keyAttributes |IResolvable \| { [string]: string } |The key attributes for the SLO | |
There was a problem hiding this comment.
Please remove IResolvable.
Also, based on the docs .. KeyAttributes seems it accepts only limited set of keys:
- Type: designates the type of object this is.
- ResourceType: specifies the type of the resource. This field is used only when the value of the Type field is Resource or AWS::Resource.
- Name: specifies the name of the object. This is used only if the value of the Type field is Service, RemoteService, or AWS::Service.
- Identifier: identifies the resource objects of this resource. This is used only if the value of the Type field is Resource or AWS::Resource.
- Environment: specifies the location where this object is hosted, or what it belongs to.
If my understanding is correct, so why not to create a new Type KeyAttributes that contain the previous properties, and make all of them optional (if some of them are required, so you can mark them as required).
| |Name |Type |Description | | ||
| |--- |--- |--- | | ||
| |name |string |The name of the SLO | | ||
| |keyAttributes |IResolvable \| { [string]: string } |The key attributes for the SLO | |
There was a problem hiding this comment.
similar to line 88 comment
|
|
||
| `feat(cloudwatch): introduce new Application Signals L2 constructs for simplifying SLO creation and management` | ||
|
|
||
| ### README |
There was a problem hiding this comment.
this the design, not readme
This is a request for comments about adding L2 constructs for simplifying Application Signals SLO process. See #673 for
additional details.
APIs are signed off by @ moelasmar.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license