-
Notifications
You must be signed in to change notification settings - Fork 321
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
NET-10124: Add metrics endpoint and labels to sync catalog deployment #4212
Conversation
|
||
if c.prometheusSink == nil { | ||
c.logger.Error("Prometheus sink not initialized, metrics cannot be displayed") | ||
c.prometheusSink = &prometheus.PrometheusSink{} |
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 need this to not throw a nil pointer error. Happy to remove
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.
will this condition ever not be nil? the default for a pointer field is nil
and I don't see where we're setting this so I'd assume we always fall into this block right?
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.
We set this value on L522, there was was nil pointer errors in testing so I put this there, I'll remove it cos it's not a condition I see us hitting in production.
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.
oh nah I think it's a valid check, what I mean is there ever a case where this would actually be set? to me it seems like this will always be nil and need to be set
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 it won't be nil, recordMetrics()
returns prometheus.PrometheusSink
which we assign to this field. So c.prometheusSink
value will only be nil if recordMetrics()
returns an error(as we just log create metrics sink error and continue running feature). Let's chat if you need further explanation
} | ||
|
||
// authorizeMiddleware validates the token and returns http handler. | ||
func (c *Command) authorizeMiddleware() func(http.Handler) http.Handler { |
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.
is this doing anything yet or is this a placeholder for when we implement auth?
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 is just a placeholder for now
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.
Some thoughts just looking through the code. Spending some time this afternoon tinkering with the new metrics in Grafana. Exciting stuff, great work!
Changes proposed in this PR
How I've tested this PR
How I expect reviewers to test this PR
Checklist