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

LogGroup#extractMetric doesn't support '/' character or tokens/derived metric namespace or names. #1351

Closed
sam-goodwin opened this issue Dec 13, 2018 · 6 comments · Fixed by #1375
Assignees
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug.

Comments

@sam-goodwin
Copy link
Contributor

LogGroup.extractMetric includes the metric namespace and name in the underlying construct name. This prohibits valid namespaces such as those that contain a /, or any derived token values such as function names etc.

const myFunc = new lambda.Function(..)
logGroup.extractMetric('$.myMetric', `mynamespace${myFunc.functionName}`, 'mymetric');

Throws error:

Error: Construct names cannot include '/': mynamespace/${Token[MyFunction.Ref.13]}_mymetric
@eladb
Copy link
Contributor

eladb commented Dec 13, 2018

Interesting. there's an attempt of extractMetric to use the namespace as a construct name, but if the name includes a token, this whole idea kind of fails.

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 13, 2018

I expected the namespace to be static while writing this, like MyApp.

@eladb
Copy link
Contributor

eladb commented Dec 13, 2018

Apparently the namespace in @sam-goodwin's case is not static. Even though, we should protect ourselves by checking if unresolved(id) when a construct ID is passed in, no?

@sam-goodwin
Copy link
Contributor Author

It also doesn't support static namespaces with a / which are valid.

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 13, 2018

Agreed on both of those.

@eladb
Copy link
Contributor

eladb commented Dec 17, 2018

What I ended up doing is basically allow "/"s to be included in construct IDs, but they are mangled to "--" as soon as the construct is created. I think in most cases that won't be horribly surprising or problematic because it's quite rare for people to actually want to find constructs by their ID. In the rare case, I've added some documentation to that effect.

eladb pushed a commit that referenced this issue Dec 17, 2018
Relax construct ID constraints to allow "/" (path seprator)
to be used in construct IDs, but convert it to "--" so it won't
collide with path strings.

It's quite rare for people to actually try to find a construct
by their ID (it's mostly "write-only") and the logical ID is eventually
mangled anyway when synthesized to CFN.

Fails if the construct ID contains a token. This won't work because
we mangle the IDs as strings when we generate the logical ID
and the construct's unique ID, and stringified tokens won't be
resolved.

Fixes #1351
Fixes #1374
@eladb eladb self-assigned this Dec 17, 2018
eladb pushed a commit that referenced this issue Dec 17, 2018
…1375)

Relax construct ID constraints to allow "/" (path seprator)
to be used in construct IDs, but convert it to "--" so it won't
collide with path strings.

It's quite rare for people to actually try to find a construct
by their ID (it's mostly "write-only") and the logical ID is eventually
mangled anyway when synthesized to CFN.

Fails if the construct ID contains a token. This won't work because
we mangle the IDs as strings when we generate the logical ID
and the construct's unique ID, and stringified tokens won't be
resolved.

Fixes #1351
Fixes #1374
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 bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants