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

Move prometheus server to main #399

Merged
merged 7 commits into from
Mar 8, 2023

Conversation

KalmanMeth
Copy link
Collaborator

Even if we have no encode_prom, define the prometheus endpoint so that operational metrics can be reported.
Define prefix for operational metrics.
Remove addr:port from encode_prom definition.
Allow multiple instantiations of encode_prom with different parameters.
All encode_prom instances use the same global prometheus endpoint.

@openshift-ci
Copy link

openshift-ci bot commented Mar 2, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from kalmanmeth. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@KalmanMeth KalmanMeth linked an issue Mar 2, 2023 that may be closed by this pull request
@KalmanMeth KalmanMeth requested review from ronensc and jotak March 2, 2023 09:57
Comment on lines 29 to 31
// StartServer listens for prometheus resource usage requests
func StartPromServer(tlsConfig *api.PromTLSConf, server *http.Server) {
logrus.Debugf("entering startServer")
Copy link
Member

Choose a reason for hiding this comment

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

nit: doc mismatch StartServer => StartPromServer, and debug log startServer => StartPromServer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

}
if err != nil && err != http.ErrServerClosed {
logrus.Errorf("error in http.ListenAndServe: %v", err)
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

I saw that you kept the same behaviour, but surely it would make more sense to not exit when metricsSettings.NoPanic is true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@jotak jotak added the breaking-change This pull request has breaking changes. They should be described in PR description. label Mar 3, 2023
@jotak
Copy link
Member

jotak commented Mar 3, 2023

Thanks @KalmanMeth for this! :)

@jotak
Copy link
Member

jotak commented Mar 3, 2023

@KalmanMeth the PR looks good to me, just a few minor remarks above.
Since it's a breaking change we must put it on hold because changes are necessary on the operator side as well
/hold

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

Merging #399 (596ac17) into main (8d5b93c) will increase coverage by 0.02%.
The diff coverage is 9.52%.

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   60.98%   61.00%   +0.02%     
==========================================
  Files          91       92       +1     
  Lines        6297     6399     +102     
==========================================
+ Hits         3840     3904      +64     
- Misses       2223     2259      +36     
- Partials      234      236       +2     
Flag Coverage Δ
unittests 61.00% <9.52%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/flowlogs-pipeline/main.go 0.00% <0.00%> (ø)
pkg/api/encode_prom.go 100.00% <ø> (ø)
pkg/config/config.go 63.33% <ø> (ø)
pkg/operational/metrics.go 67.10% <0.00%> (-0.90%) ⬇️
pkg/pipeline/encode/encode_prom.go 77.72% <ø> (-0.06%) ⬇️
pkg/pipeline/utils/prom_server.go 0.00% <0.00%> (ø)
pkg/confgen/flowlogs2metrics_config.go 68.83% <100.00%> (+0.83%) ⬆️
pkg/pipeline/extract/conntrack/store.go 79.11% <0.00%> (-5.06%) ⬇️
pkg/pipeline/extract/timebased/heap.go 95.65% <0.00%> (-4.35%) ⬇️
pkg/api/conntrack.go 88.81% <0.00%> (-0.87%) ⬇️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

lgtm
restarted the e2e test that failed with a timeout

@openshift-ci openshift-ci bot added the lgtm label Mar 7, 2023
@KalmanMeth KalmanMeth merged commit 04f8a14 into netobserv:main Mar 8, 2023
@jotak
Copy link
Member

jotak commented Mar 8, 2023

@KalmanMeth I mentioned this shouldn't be merged before the operator PR is ready to address the breaking changes; currently operator deployment is broken, so we need to do either a quick fix on the operator side, or rollback this one

@KalmanMeth
Copy link
Collaborator Author

@jotak Sorry for the misunderstanding. I saw that you had removed the the no-merge label and added the label lgtm.

@jotak
Copy link
Member

jotak commented Mar 8, 2023

the do-not-merge label is still there :)
no worries .. should be easy to fix in the operator

jotak added a commit to jotak/network-observability-operator that referenced this pull request Mar 8, 2023
jotak added a commit to jotak/network-observability-operator that referenced this pull request Mar 8, 2023
openshift-merge-robot pushed a commit to netobserv/network-observability-operator that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This pull request has breaking changes. They should be described in PR description. do-not-merge/hold lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple reporting of operational metrics from network metrics
2 participants