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

MetricFilter should expose its metric definition via a metric property. #1353

Closed
sam-goodwin opened this issue Dec 13, 2018 · 5 comments · Fixed by #8556
Closed

MetricFilter should expose its metric definition via a metric property. #1353

sam-goodwin opened this issue Dec 13, 2018 · 5 comments · Fixed by #8556
Assignees
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.

Comments

@sam-goodwin
Copy link
Contributor

sam-goodwin commented Dec 13, 2018

Say you create a MetricFilter:

new logs.MetricFilter(this, 'MetricFilter', {
  filterPattern: logs.FilterPattern.exists('$.myMetric'),
  metricNamespace,
  metricName,
  metricValue: '$.myMetric',
  logGroup
});

This filter should expose a property for the metric it defines.

// I want to call
const metric = metricFilter.metric()

// instead of
const metric = new cloudwatch.Metric({
  metricName,
  namespace: metricNamespace,
  statistic: cloudwatch.Statistic.Maximum
})
@eladb
Copy link
Contributor

eladb commented Dec 13, 2018

👍 Our convention is to expose metric() methods on AWS constructs.

@eladb eladb self-assigned this Aug 12, 2019
@eladb eladb assigned rix0rrr and unassigned eladb Sep 3, 2019
@rix0rrr rix0rrr added the feature-request A feature should be added or improved. label Sep 27, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 27, 2019

Yep, that's a good idea.

@rix0rrr rix0rrr added the effort/small Small work item – less than a day of effort label Jan 23, 2020
@michaelday008
Copy link

While this change is being made, is it possible to link the metric filter to the alarm?

In the UI, I can look at any metric filter on a log group, and click the "create alarm" button and then when looking at the log filters, the attached alarm will show.

Using CDK, this seems to be created in a disconnected fashion so when I go into the UI and look at the metric filter it doesn't show the alarm. I have to go to the alarms page and see them and it can be difficult to make the connection that this metric filter will result in an alarm.

@michaelwiles
Copy link
Contributor

So I've run into this "issue" as well...

Fundamentally it's a bit clunky that in order to create a metric when you have a metric filter you make sure that metric names are the same.

This is the non intuitive or hard to figure out aspect of how it is now.

It would be nice to have some more explicit solution though I'm not sure what it would look like...

Part of the challenge is that in order to create a metric from a metric filter you need a bit more information. i.e. you need a statistic - metric filter does not have this.

So I would propose adding a method to metric say, "from_metric_fitler(metric_filter, ...)" and then include the extra info needed to create a metric from a metric filter.

I'm fairly confident this is possible at the type script level...

I would say it should rather be on metric than on metric filter...

@rix0rrr rix0rrr added the good first issue Related to contributions. See CONTRIBUTING.md label Jun 8, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 8, 2020

I would recommend a getter instead of a method.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jun 17, 2020
@mergify mergify bot closed this as completed in #8556 Jun 18, 2020
mergify bot pushed a commit that referenced this issue Jun 18, 2020
It is convention to expose metrics on AWS constructs, but metric filters currently do not.

This PR adds a `metric()` API that creates a metric from a metric filter with the same name and namespace. The user can specify what statistic to use, or it will default to average over 5 minutes.

fixes #1353 

----

*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
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants