-
Notifications
You must be signed in to change notification settings - Fork 171
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
Add Static labels #50
Conversation
26709aa
to
b04044a
Compare
config/config.go
Outdated
TypeString string `yaml:"type"` // the Prometheus metric type | ||
Help string `yaml:"help"` // the Prometheus metric help text | ||
KeyLabels []string `yaml:"key_labels,omitempty"` // expose these columns as labels from SQL | ||
StaticLabels []StaticLabel `yaml:"static_labels,omitempty"` // expose these columns as static labels |
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.
I'd rather use a map[string]string
, similar to StaticConfig.Labels
. Then you won't run into any issues due to duplicate labels, as the YAML parser will simply take the last value. As is, you'll likely get a cryptic start-up error when creating the metric family.
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.
It also looks better to me, but I thought it was already agreed. Will change.
config/config.go
Outdated
TypeString string `yaml:"type"` // the Prometheus metric type | ||
Help string `yaml:"help"` // the Prometheus metric help text | ||
KeyLabels []string `yaml:"key_labels,omitempty"` // expose these columns as labels from SQL | ||
StaticLabels []StaticLabel `yaml:"static_labels,omitempty"` // expose these columns as static labels |
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.
Also, these are no (result set) columns, they're simply static constants. The comment should be something like "constant/fixed label-value pairs".
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.
I thought that the implementation it was more or less agreed, so I picked the initial contributor commit as is and added the missing bit. :) I'll work this out. 👍
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 putting up with me.
Apparently I didn't look too close at the original PR. I'm not a Go programmer by day, the only reason I wrote this in Go is that Prometheus is written in Go and some of the underlying libraries could be reused.
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.
Just based upon the number of docker image pulls (~1.4m) you did a really good job, thanks for that! I recently joined the team and sql_exporter
is used there across many environments. The best thing I can do personally is to contribute to the upstream. :)
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.
Well, I tried. I would have used anything that was already there, but couldn't find anything half-decent for MSSQL, so I had to roll my own. Plus I needed to be able to run arbitrary queries.
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.
👍
metric.go
Outdated
@@ -50,9 +50,20 @@ func NewMetricFamily(logContext string, mc *config.MetricConfig, constLabels []* | |||
labels = append(labels, mc.ValueLabel) | |||
} | |||
|
|||
// Create a copy of original slice to apply sort properly | |||
sortableLabels := append(constLabels[:0:0], constLabels...) |
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.
This will append the slice to itself, it doesn't change anything. It should instead be:
sortableLabels := append(constLabels[:0:0], constLabels...) | |
sortedLabels := append([]int(nil), constLabels...) |
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.
Yeah, it makes sense. I just came across the suggestion here and I like the guarantees we have with that, so decided to try something new and also get your opinion about it.
It gives me the same result if I'm not missing anything: https://play.golang.org/p/rs0LoTAI8xY
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.
You're right, of course, please leave it as is. My apologies.
I did try your code in the playground, but I must have appended the value directly to the [:0:0]
slice instead of appending the contents of the slice itself first.
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.
No worries, thanks for double checking!
metric.go
Outdated
@@ -50,9 +50,20 @@ func NewMetricFamily(logContext string, mc *config.MetricConfig, constLabels []* | |||
labels = append(labels, mc.ValueLabel) | |||
} | |||
|
|||
// Create a copy of original slice to apply sort properly |
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.
// Create a copy of original slice to apply sort properly | |
// Create a copy of original slice to avoid modifying constLabels |
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.
👍
b04044a
to
dc137b9
Compare
@free I've applied the changes you suggested, now it works exactly as we discussed:
we get the following result (database values are correct):
|
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.
LGTM, thanks.
Hi @free ,
It seems like the previous pull request was abandoned, though the idea is good.
I checked your previous comments and added what was missing.
What's your opinion?