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

Add service monitor for flyte admin and propeller service #4427

Merged

Conversation

vraiyaninv
Copy link
Contributor

Describe your changes

Service monitors are a common way to auto-discover service targets dynamically in Prometheus.

This change allows the creation of a service monitor for the admin service as well as the creation of a service and service monitor for the propeller.

Propeller service allows discovering multiple instances of propeller pods and exporting its metrics.

Admin service monitor and propeller service as well as its service monitor are disabled by default

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Setup Process

Screenshots

Note to reviewers

Related PRs

Copy link

welcome bot commented Nov 15, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

charts/flyte-core/templates/admin/service-monitor.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
{{- if and .Values.flytepropeller.enabled .Values.flytepropeller.service.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be only enabled when serviceMonitor is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can still have a service without servicemonitor so there is no harm in creating just the service. This keeps it flexible to have the service for any other ports which may not be related to prometheus port.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (956b24e) 59.81% compared to head (f77cd93) 59.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4427   +/-   ##
=======================================
  Coverage   59.81%   59.82%           
=======================================
  Files         632      632           
  Lines       53644    53644           
=======================================
+ Hits        32089    32091    +2     
+ Misses      19029    19028    -1     
+ Partials     2526     2525    -1     
Flag Coverage Δ
unittests 59.82% <ø> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidmirror-ops
Copy link
Contributor

Propeller service allows discovering multiple instances of propeller pods

This is particular to multi-cluster deployments correct?

@vraiyaninv
Copy link
Contributor Author

Propeller service allows discovering multiple instances of propeller pods

This is particular to multi-cluster deployments correct?

It is for a single cluster, when you have multiple pods with propeller manager. We could have a pod monitor as well, but for uniformity across services, I leaned towards creating a service and having a service monitor.

@davidmirror-ops davidmirror-ops self-requested a review November 20, 2023 19:33
@davidmirror-ops davidmirror-ops merged commit 568e686 into flyteorg:master Nov 20, 2023
47 checks passed
Copy link

welcome bot commented Nov 20, 2023

Congrats on merging your first pull request! 🎉

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.

3 participants