Fix prometheus exporter when tags are missing. #989
Conversation
|
/cc @johanbrandhorst |
There was a problem hiding this comment.
Thanks for working on this @bogdandrutu! I am doing the first pass, but please revert the removal of tagsToLabels since Prometheus needs sanitization of keys e.g. "org/foo" should become "org_foo" but this change removes that.
exporter/prometheus/prometheus.go
Outdated
| func tagsToLabels(tags []tag.Tag) []string { | ||
| var names []string | ||
| for _, tag := range tags { | ||
| names = append(names, internal.Sanitize(tag.Key.Name())) |
There was a problem hiding this comment.
Why did you remove this? It is imperative that we sanitize keys for Proemtheus otherwise "org/foo" will fail for Prometheus which expects "org_foo".
There was a problem hiding this comment.
This is unused as you can see I did not change any code that calls it, and this seems to be a package private function.
There was a problem hiding this comment.
Gotcha, but it is noisy in this PR and unrelated to this change.
There was a problem hiding this comment.
I see your point, but dead code is the thing that I "like" the most :) happy to do a different PR if you feel strong.
There was a problem hiding this comment.
Reverted and I would send a different PR.
| defer view.Unregister(v) | ||
| view.SetReportingPeriod(time.Millisecond) | ||
| // Make a measure without some tags in the view. | ||
| ctx, _ := tag.New(context.Background(), tag.Upsert(k2, "issue659"), tag.Upsert(k4, "issue659")) |
There was a problem hiding this comment.
For a fully defensible test, could you please add here a tag that wasn't registered with the view? e.g. k6 and k7?
There was a problem hiding this comment.
Thank you @bogdandrutu! LGTM with one minor suggestion to add keys(in the test) that weren't registered with the view, and that will help test out the placement logic and ensure that it doesn't read out of bounds.
we need this bug fix: census-instrumentation/opencensus-go#989
we need this bug fix: census-instrumentation/opencensus-go#989
Fixes #659