stackdriver_exporter icon indicating copy to clipboard operation
stackdriver_exporter copied to clipboard

Sanitize metric type prefixes

Open sysedwinistrator opened this issue 1 year ago • 5 comments

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

Fixes https://github.com/prometheus-community/stackdriver_exporter/issues/103

sysedwinistrator avatar Mar 19 '24 18:03 sysedwinistrator

hi there. is it going to be merged soon?

jotka avatar Jun 13 '24 04:06 jotka

It's still in draft, is it ready for review?

SuperQ avatar Jun 13 '24 05:06 SuperQ

@sysedwinistrator will you release it from draft status?

jotka avatar Jun 13 '24 06:06 jotka

@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 stackdriver_exporter_test.go?

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 :)

sysedwinistrator avatar Jun 13 '24 10:06 sysedwinistrator

@SuperQ it's ready to review now :)

jotka avatar Jun 28 '24 12:06 jotka

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.

sysedwinistrator avatar Aug 22 '24 14:08 sysedwinistrator

What is ithe status of this PR?

srclosson avatar Aug 28 '24 14:08 srclosson

I'm blocked from using the exporter right now...waiting for this PR to hopefully fix that 🤞

4wdonny avatar Aug 28 '24 15:08 4wdonny

What is ithe status of this PR?

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.

sysedwinistrator avatar Aug 29 '24 08:08 sysedwinistrator

@kgeckhart will there be a release with this change soon?

initharrington avatar Oct 04 '24 14:10 initharrington