Skip to content
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

feat(cloudwatch): Widgets can define start and end times, including relative values #26969

Merged
merged 14 commits into from
Sep 19, 2023

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Sep 1, 2023

This PR adds start and end properties to Widgets.

These properties can be used to specify the time range for each graph widget independently from those of the dashboard.
The parameters can be specified at GraphWidget, GaugeWidget, and SingleValueWidget.

Closes #26945.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team September 1, 2023 08:19
@github-actions github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Sep 1, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation dismissed their stale review September 1, 2023 10:23

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@go-to-k go-to-k marked this pull request as ready for review September 1, 2023 10:50
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 1, 2023
@go-to-k go-to-k changed the title feat(cloudwatch): add start and end properties to GraphWidgets feat(cloudwatch): add start and end properties to Widgets Sep 1, 2023
*
* @default When the dashboard loads, the start time will be the default time range.
*/
readonly start?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i don't love the idea that we're typing these values as string (both for start and end).

can you either:

a) point me to a place elsewhere in aws-cdk-lib where we already type something similar as a string.
b) help me design a better type system that can then generate the format that cloudwatch wants

Copy link
Contributor Author

@go-to-k go-to-k Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaizencc

I can certainly see that.

a. The start and end are already used in Dashboard.

/**
* The start of the time range to use for each widget on the dashboard.
* You can specify start without specifying end to specify a relative time range that ends with the current time.
* In this case, the value of start must begin with -P, and you can use M, H, D, W and M as abbreviations for
* minutes, hours, days, weeks and months. For example, -PT8H shows the last 8 hours and -P3M shows the last three months.
* You can also use start along with an end field, to specify an absolute time range.
* When specifying an absolute time range, use the ISO 8601 format. For example, 2018-12-17T06:00:00.000Z.
*
* @default When the dashboard loads, the start time will be the default time range.
*/
readonly start?: string;
/**
* The end of the time range to use for each widget on the dashboard when the dashboard loads.
* If you specify a value for end, you must also specify a value for start.
* Specify an absolute time in the ISO 8601 format. For example, 2018-12-17T06:00:00.000Z.
*
* @default When the dashboard loads, the end date will be the current time.
*/
readonly end?: string;

b. For example, I came up with a way to create a private constructor and a static method for each type.

export class StartTimeRange {
  public static fromMinutes(start: number): StartTimeRange {
    if (start <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange(`-PT${start}M`);
  }

  public static fromHours(start: number): StartTimeRange {
    if (start <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange(`-PT${start}H`);
  }

  public static fromDays(start: number): StartTimeRange {
    if (start <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange(`-P${start}D`);
  }

  public static fromWeeks(start: number): StartTimeRange {
    if (start <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange(`-P${start}W`);
  }

  public static fromMonths(start: number): StartTimeRange {
    if (start <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange(`-P${start}M`);
  }

  public static fromDate(start: Date): StartTimeRange {
    return new StartTimeRange(start.toISOString());
  }

  private constructor(public readonly start: string) { }
}

export class EndTimeRange {
  public static fromDate(end: Date): EndTimeRange {
    return new EndTimeRange(end.toISOString());
  }

  private constructor(public readonly end: string) { }
}

Copy link
Contributor Author

@go-to-k go-to-k Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b-2. Or I thought of this too. By creating StartTimeRange interface and separating into unit and date, the combination of end and start can be validated. (In the case of the above b example, the validation can be implemented to check whether starts with '-P' as string.)

e.g. If end is specified, start must specify date instead of unit.

export interface StartTimeRangeType {
  unit?: string; // ex. -PT5M, -P3M.
  date?: string; // ISO 8601 format. For example, 2018-12-17T06:00:00.000Z.
}

export class StartTimeRange {
  public static fromMinutes(unit: number): StartTimeRange {
    if (unit <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange({
      unit: `-PT${unit}M`,
    });
  }

  public static fromHours(unit: number): StartTimeRange {
    if (unit <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange({
      unit: `-PT${unit}H`,
    });
  }

  public static fromDays(unit: number): StartTimeRange {
    if (unit <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange({
      unit: `-P${unit}D`,
    });
  }

  public static fromWeeks(unit: number): StartTimeRange {
    if (unit <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange({
      unit: `-P${unit}W`,
    });
  }

  public static fromMonths(unit: number): StartTimeRange {
    if (unit <= 0) {
      throw new Error('start must be greater than 0');
    }
    return new StartTimeRange({
      unit: `-P${unit}M`,
    });
  }

  public static fromDate(date: Date): StartTimeRange {
    return new StartTimeRange({
      date: date.toISOString(),
    });
  }

  private constructor(public readonly start: StartTimeRangeType) {}
}

export class EndTimeRange {
  public static fromDate(date: Date): EndTimeRange {
    return new EndTimeRange(date.toISOString());
  }

  private constructor(public readonly end: string) { }
}

export class GaugeWidget extends Resource {
  // ...
  // ...
  constructor(scope: Construct, id: string, props: GaugeWidgetProps = {}) {
    // ...
    // ...
    if (props.end !== undefined && props.start === undefined) {
      throw new Error('If you specify a value for end, you must also specify a value for start.');
    }
    if (props.end !== undefined && props.start?.start.date === undefined) {
      throw new Error('If `end` is specified, `start` must be specified by `StartTimeRange.fromDate`.');

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a chat with @kaizencc

Given that we already use a string elsewhere, and that it might be quite an effort to provide a complete implementation of this rather complex standard, using string will be fine.

Users can always fallback to native language features and provide something like new Date().toISOString()

mrgrain
mrgrain previously approved these changes Sep 19, 2023
@mrgrain mrgrain changed the title feat(cloudwatch): add start and end properties to Widgets feat(cloudwatch): Widget can have start and end times Sep 19, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mrgrain mrgrain changed the title feat(cloudwatch): Widget can have start and end times feat(cloudwatch): Widgets can have start and end times Sep 19, 2023
@mrgrain mrgrain changed the title feat(cloudwatch): Widgets can have start and end times feat(cloudwatch): Widgets can define start and end times Sep 19, 2023
@mrgrain mrgrain changed the title feat(cloudwatch): Widgets can define start and end times feat(cloudwatch): Widgets can define start and end times, including relative values Sep 19, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 19, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

kaizencc
kaizencc previously approved these changes Sep 19, 2023
@mergify mergify bot dismissed stale reviews from kaizencc and mrgrain September 19, 2023 17:29

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 115265c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 2866240 into aws:main Sep 19, 2023
9 checks passed
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

HBobertz pushed a commit that referenced this pull request Sep 19, 2023
…ng relative values (#26969)

This PR adds `start` and `end` properties to Widgets.

These properties can be used to specify the time range for each graph widget independently from those of the dashboard.
The parameters can be specified at `GraphWidget`, `GaugeWidget`, and `SingleValueWidget`.

Closes #26945.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-cloudwatch): Ability to set time frames for individual graphs in a dashboard
4 participants