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

fix: enable ctr for prometheus reporting #5362

Closed
wants to merge 3 commits into from

Conversation

klopfdreh
Copy link
Contributor

Currently the CTR does not report to prometheus-rsocket-proxy - even if the environment variables are given from the SCDF server. With this fix the CTR is reporting to the prometheus-rsocket-proxy like all other task applications

@klopfdreh
Copy link
Contributor Author

@cppwfs - I think there are still references of the build which points to the spring repository instead of central.

image

@corneil
Copy link
Contributor

corneil commented Jun 1, 2023

@cppwfs - I think there are still references of the build which points to the spring repository instead of central.

image

@cppwfs - I think there are still references of the build which points to the spring repository instead of central.

image

The latest commit contains a fix for the doc-resources issues.

@klopfdreh
Copy link
Contributor Author

klopfdreh commented Jun 1, 2023

This issue resolves #5357 .

No this issue is not the same.

This PR solves the issue that the CTR sends the metrics to the Prometheus Proxy as the dependencies were only scope test which was wrong.

The issue linked by you occurs if you scale up the replicas of SCDF to 2 or higher. When you do so there are two instances of the SCDF server reporting the values to prometheus with the same application. So one overrides the values of the other. In this case the dashboard needs to be adjusted. In the issue itself I mentioned how to fix this.

@cppwfs
Copy link
Contributor

cppwfs commented Jun 1, 2023

Whoops! You are correct. I'll remove my comment from before to remove confusion.

@corneil
Copy link
Contributor

corneil commented Jun 1, 2023

The docresources is still and issue. I thought the changes was in.
Creating a PR for it now.

@corneil corneil requested a review from cppwfs June 1, 2023 15:58
@cppwfs
Copy link
Contributor

cppwfs commented Jun 2, 2023

Probably need to add an application.properties where Prometheus is disabled by default. i.e.
management.metrics.export.prometheus.rsocket.enabled=false
management.metrics.export.prometheus.enabled=false
This way users who do use Prometheus can enable it when they need it.

@klopfdreh
Copy link
Contributor Author

As far as I know those properties are disabled by default, but given when ever Prometheus RSocket Proxy is enabled in the SCDF.

I think separate properties are not needed.

@onobc
Copy link
Contributor

onobc commented Jun 2, 2023

Thanks @cppwfs and @klopfdreh. They are indeed disabled by default here.

I have not caught up on this thread but will digest shortly to see if I have anything to add.

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

LGTM @klopfdreh - thanks for another excellent contribution!

@onobc
Copy link
Contributor

onobc commented Jun 5, 2023

Closed via 5df193a

@onobc onobc closed this Jun 5, 2023
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