-
Notifications
You must be signed in to change notification settings - Fork 108
Sanitize metric type prefixes #319
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
Sanitize metric type prefixes #319
Conversation
When more than one prefix matches the same metric descriptor, this will throw the error "collected metric xxx was collected before with the same name and label values". For example, using the metric type prefixes foo.googleapis.com/bar (a prefix) and foo.googleapis.com/bar/baz (a metric) will result in an error because both match the metric foo.googleapis.com/bar/baz. Further, using the metric type prefixes foo.googleapis.com/bar/baz (a metric) and foo.googleapis.com/bar/baz_count (a metric) will result in an error because both match the metric foo.googleapis.com/bar/baz_count. While the first pitfall could be expected by the user, the latter will come as a complete surprise to anyone who is not aware that stackdriver-exporter internally uses an MQL query in the form of metric.type = starts_with("<prefix>") to filter the metrics. Avoid this by sanitizing the provided metric type prefixes in the following way: - Drop any duplicate prefixes - Sort the prefixes (required by the next step) - Drop any prefixes that start with another prefix present in the input Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
4a82b2e
to
b3247d4
Compare
hi there. is it going to be merged soon? |
It's still in draft, is it ready for review? |
@sysedwinistrator will you release it from draft status? |
Sorry, I had completely forgotten about this PR. In the project I'm working on we already have to merge metric prefixes from multiple sources so we just worked around this issue by deduplicating the prefixes where we merge them. I'd like to add a simple test for this function. I guess I can just add a a Also, as I learned when working on our internal solution, the second case (previous prefix starts with, i.e. is longer than, the current prefix) will never happen in an alphanumerically sorted list :) |
In alphanumerically sorted list of strings, abcdef will never come before abc. Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
676c67a
to
8e85d13
Compare
Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
8e85d13
to
4d76015
Compare
@SuperQ it's ready to review now :) |
Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
6d10539
to
c46b249
Compare
Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
Signed-off-by: Edwin Mackenzie-Owen <[email protected]>
I've also changed the code to initialize the return array in the beginning of the function (to ensure it doesn't return a nil array if the input is an empty array). Because of that the return array doesn't need to be intiialized in the first iteration of the for loop anymore, so I was also able to simplify the if condition. |
What is ithe status of this PR? |
I'm blocked from using the exporter right now...waiting for this PR to hopefully fix that 🤞 |
It needs further review. I don't think there any blockers. It just took me a while after the initial review to get back to this PR and address the open issues. |
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.
Thanks for this change!
I don't think this fully fixes #103 though but dramactially cut down the instances of it caused by overlapping prefixes. This case specifically is interesting #103 (comment) where they are using two non-overlapping prefixes but something about the data being returned from GCP APIs is causing duplicates.
@kgeckhart will there be a release with this change soon? |
When more than one prefix matches the same metric descriptor, this will throw the error
collected metric xxx was collected before with the same name and label values
.For example, using the metric type prefixes
foo.googleapis.com/bar (a prefix)
and
foo.googleapis.com/bar/baz (a metric)
will result in an error because both match the metric
foo.googleapis.com/bar/baz
.Further, using the metric type prefixes
foo.googleapis.com/bar/baz (a metric)
and
foo.googleapis.com/bar/baz_count (a metric)
will result in an error because both match the metric
foo.googleapis.com/bar/baz_count
.While the first pitfall could be expected by the user, the latter will come as a complete surprise to anyone who is not aware that stackdriver-exporter internally uses an MQL query in the form of
metric.type = starts_with("<prefix>")
to filter the metrics.
Avoid this by sanitizing the provided metric type prefixes in the following way:
Fixes #103