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

Associator buildLabelsMap to lower case EndpointName to match ARN #1178

Conversation

GGonzalezGomez
Copy link
Contributor

This is related to #1163. The problem we are facing is that when the matching resource is being looked up, the associator is not finding it. The signature is calculated using the ARN endpoint name through the regex, but the ARN endpoint name is in lower case. If the endpoint name contains any upper case characters it fails as the signature value is different from the ARN one due to casing.
So, in order to fix it if the metric is from sagemaker and it uses the endpoint name, if it contains any upper case character it converts the endpoint name to lower case to calculate the signature.
In the issue some example is provided, hopefully this way it can be fixed.

// endpoint name value to be able to match the resource ARN
if cwMetric.Namespace == "AWS/SageMaker" && name == "EndpointName" {
if awsSageMakerEndpointUpper.MatchString(value) {
value = strings.ToLower(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we can always just convert to lowercase without the regex match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I added it last minute to match the AmazonMQ one. I thought it would be more efficient to just convert to lowercase when needed but after some tests it is more efficient to just run the conversion every time.
I'll remove that check asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regex has been removed now

@cristiangreco cristiangreco merged commit c3a81ef into prometheus-community:master Oct 16, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants