Skip to content
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

Metrics configuration #(1/4) #525

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Metrics configuration #(1/4) #525

merged 1 commit into from
Jun 16, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Jun 15, 2021

Related to #124

I created a MetricsAddress field. The default value is “0.0.0.0:8080" and defines where Prometheus metrics endpoint is listening. (see: https://docs.openshift.com/container-platform/4.1/applications/operator_sdk/osdk-monitoring-prometheus.html#osdk-monitoring-prometheus-metrics-helper_osdk-monitoring-prometheus)

I also removed flag.StringVar "metrics-addr", ":8080”, from main.go as moving inputs into depresolver and also, removed enableLeaderElection and overall flags completely.

Signed-off-by: kuritka kuritka@gmail.com

@kuritka kuritka changed the title Metrics configuration Metrics configuration #1 Jun 15, 2021
@k0da
Copy link
Collaborator

k0da commented Jun 15, 2021

@kuritka this is a bit confusing. 9090 is a default p8s server port. 8080 and MetricsAddr was pretty self explanatory term.

@kuritka
Copy link
Collaborator Author

kuritka commented Jun 15, 2021

@kuritka this is a bit confusing. 9090 is a default p8s server port. 8080 and MetricsAddr was pretty self explanatory term.

Ok, will change to 8080. What do you mean by MetricsAddr ?

@k0da
Copy link
Collaborator

k0da commented Jun 15, 2021

@kuritka you have it now as Prometheus.Port (sounds like server port), MetricsAddr defines metrics endpoint better IMO

@kuritka kuritka force-pushed the prometheus2 branch 3 times, most recently from bf4a0c5 to f3e9075 Compare June 15, 2021 16:03
@kuritka
Copy link
Collaborator Author

kuritka commented Jun 15, 2021

@kuritka you have it now as Prometheus.Port (sounds like server port), MetricsAddr defines metrics endpoint better IMO

@k0da , good point 👍 . I refactored config and tests. Now everything seems to be ok.

@kuritka kuritka force-pushed the prometheus2 branch 2 times, most recently from ad6e895 to 92f5a43 Compare June 15, 2021 16:10
k0da
k0da previously approved these changes Jun 15, 2021
Copy link
Collaborator

@k0da k0da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Port: 9443,
LeaderElection: enableLeaderElection,
LeaderElection: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we never tested it, but what if somebody would like to try to execute multiple k8gb controllers with leader election mechanism in the future? currently it is optional but here it is becoming hardcoded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enableLeaderElection was accessible through an argument of executable. Making such functionality available would mean passing the argument to the controller pod. Till now it was always false anyway.

What we can do is create an issue, expose an environment variable and test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuritka ok let's log the issue then

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #526

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even more, I think it requires to have locking to be implemented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point @k0da. Feel free and add your comments by #526 please.

Related to #124
I created a MetricsAddress field.  The default port value is “0.0.0.0:8080" and defines where Prometheus metrics endpoint is listening. (see: https://docs.openshift.com/container-platform/4.1/applications/operator_sdk/osdk-monitoring-prometheus.html#osdk-monitoring-prometheus-metrics-helper_osdk-monitoring-prometheus)

I also removed flag.StringVar `"metrics-addr", ":8080”,` from `main.go` as moving inputs into depresolver and also, removed enableLeaderElection and overall flags completely.

Signed-off-by: kuritka <kuritka@gmail.com>
@kuritka kuritka changed the title Metrics configuration #1 Metrics configuration #(1/3) Jun 16, 2021
@kuritka kuritka changed the title Metrics configuration #(1/3) Metrics configuration #(1/4) Jun 16, 2021
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuritka LGTM

@kuritka kuritka merged commit e0dab18 into master Jun 16, 2021
@kuritka kuritka deleted the prometheus2 branch June 16, 2021 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants