-
Notifications
You must be signed in to change notification settings - Fork 4k
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(aws-cloudwatch): query results widget #7114
feat(aws-cloudwatch): query results widget #7114
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
/** | ||
* Log group to query | ||
*/ | ||
readonly logGroup: 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.
Feels like this should accept an ILogGroup
.
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.
Ok I'll check that out, I'm pretty new with CDK
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 would add a cyclic dependency:
lerna ERR! ECYCLE @aws-cdk/aws-cloudwatch -> @aws-cdk/aws-logs -> @aws-cdk/aws-cloudwatch
So I'm think that renaming it to logGroupName
and keeping it a string would be the best course of action
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 really really like users to be able to pass the object though. We will have to follow the existing dependency arrows while coming up with our solution. I'm thinking something like this:
// cloudwatch
interface IQueryLogGroup {
public readonly logGroupName: string;
}
// logs
interface ILogGroup extends cloudwatch.IQueryLogGroup {
// ...
}
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.
Ok so I rebased and added an IQueryLogGroup
.
Is the ILogGroup extends cloudwatch.IQueryLogGroup
required? For TypeScript an ILogGroup
can be supplied where an IQueryLogGroup
is required without this inheritance (it is sort of an ugly dependency - I don't know if such a thing already exists in CDK).
Perhaps it is required for other languages using JSII etc? It might be a fair trade off to not add this inheritance (until the full solution is decided). I'm thinking that would only impact users who are using more type strict languages like Java but even so an anonymous class could still work.
The other option is to make QueryWidgetProps#logGroup
take an IQueryLogGroup
or a string
and at least then with TypeScript you can pass the ILogGroup
and in other languages you still have the option to pass the name directly as a string
?
Let me know which you prefer and I will make the change.
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.
Perhaps it is required for other languages using JSII etc?
Exactly. This class could not be used in typed languages that use nominal subtyping (Java, C#), which would be a shame. Treating them as second class citizens (by forcing them to custom-implement an anonymous subclass) is not really acceptable in my opinion. It's not very discoverable either, and would present a big hurdle to adoption.
I agree with you that this solution is not fantastic, but it's at least pragmatic and works well enough until we have better alternatives.
The other option is to make
QueryWidgetProps#logGroup
take anIQueryLogGroup
or astring
You are thinking a union type here, which we as a rule don't use in L2 constructs because they won't translate well to Go.
I can understand you feeling dissatisfied with the available options. If it helps, I will take all of the design blame :).
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 mean I don't personally mind; you have the bigger picture here. So will I go with what you originally proposed?
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.
Yes please
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.
Done and rebased
(sorry about the noise below; I'm having issues building the latest version for some reason)
3a819db
to
f7c21e3
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
f7c21e3
to
c862e3c
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
c862e3c
to
d33df6b
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
```ts | ||
dashboard.addWidgets(new QueryWidget({ | ||
logGroup: LogGroup.fromLogGroupName(stack, 'MyLogGroup', 'my-log-group'), | ||
query: `fields @message |
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.
One more thing, can you rename query
to queryString
? I feel it will help us in the future if we ever want to provide query builder objects, which can then be used as query: Query.fromFields(...)
or whatever.
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.
Done!
d33df6b
to
9507731
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
9507731
to
23e93ca
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
23e93ca
to
300a79f
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Add a `QueryWidget` to generate a dashboard widget showing the results of a query from Logs Insights. This was a missing feature which is available in the console. Also make `ILogGroup` extend `cloudwatch.IQueryLogGroup` so it can be passed as a property for a `QueryWidget`. closes aws#3681
300a79f
to
867de12
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
You know what? You were right the first time. Just specifying the log group names makes way more sense. My apologies for making you change this. I added in a convenience property for specifying the query string as well, because that whitespace sensitivity is something we can make a lot easier :). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Awesome, no worries. I'm using |
You might want to add the logGroupName I moved out of ILogGroup back in ;) (I moved it to the IQueryLogGroup) |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Commit Message
feat(cloudwatch): LogGroup Query Widget
Add a
LogQueryWidget
to generate a dashboard widgetshowing the results of a query from Logs Insights.
This was a missing feature which is available in
the console.
closes #3681
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license