Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

feat(metrics): Decouple Grafana and Prometheus installation for OSM install command #1648

Merged
merged 9 commits into from
Sep 9, 2020
Merged

feat(metrics): Decouple Grafana and Prometheus installation for OSM install command #1648

merged 9 commits into from
Sep 9, 2020

Conversation

nshankar13
Copy link
Contributor

@nshankar13 nshankar13 commented Aug 31, 2020 •
edited
Loading

Remove the "enableMetricStack" flag for the OSM install command. Replace with two separate flags, enable-grafana and enable-prometheus to decouple installation of Grafana and Prometheus

Please mark with X for applicable areas.

  • New Functionality [ ]
  • Documentation [ ]
  • Install [X]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests / CI System [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

@nshankar13 nshankar13 requested a review from a team as a code owner August 31, 2020 17:12
@ghost
Copy link

ghost commented Aug 31, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

snehachhabria
snehachhabria reviewed Aug 31, 2020
View reviewed changes
charts/osm/values.yaml Outdated
Comment on lines 35 to 36
enableMetricsStack: true
enableGrafana: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have a separate flag for grafana the enableMetricsStack is primarily used for Prometheus deployment, maybe we should change that flag as well to make it more explicit

@eduser25 as FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, agree with Sneha, at this point let's have independant toggles for Grafana / Prometheus deployments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sounds good!

snehachhabria
snehachhabria reviewed Sep 1, 2020
View reviewed changes
charts/osm/templates/grafana-configmap.yaml Outdated
@@ -1,4 +1,4 @@
{{- if .Values.OpenServiceMesh.enableMetricsStack }}
{{- if and .Values.OpenServiceMesh.enableMetricsStack .Values.OpenServiceMesh.enableGrafana}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- if and .Values.OpenServiceMesh.enableMetricsStack .Values.OpenServiceMesh.enableGrafana}}
{{- if and .Values.OpenServiceMesh.enablePrometheus .Values.OpenServiceMesh.enableGrafana}}

charts/osm/values.yaml Outdated
@@ -33,6 +33,7 @@ OpenServiceMesh:
enableBackpressureExperimental: false
enableEgress: false
enableMetricsStack: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to enablePrometheus

ksubrmnn
ksubrmnn reviewed Sep 4, 2020
View reviewed changes
cmd/cli/install.go Outdated Show resolved Hide resolved
ksubrmnn
ksubrmnn reviewed Sep 4, 2020
View reviewed changes
cmd/cli/install_test.go Outdated Show resolved Hide resolved
ksubrmnn
ksubrmnn reviewed Sep 4, 2020
View reviewed changes
cmd/cli/install_test.go Outdated Show resolved Hide resolved
@shashankram
Copy link
Member

@nshankar13 Could you kindly update the PR and commit title and description.

@nshankar13 nshankar13 changed the title grafana-installation decouple Grafana and Prometheus installation for OSM install command Sep 8, 2020
@nshankar13 nshankar13 changed the title decouple Grafana and Prometheus installation for OSM install command Decouple Grafana and Prometheus installation for OSM install command Sep 8, 2020
@ksubrmnn ksubrmnn changed the title Decouple Grafana and Prometheus installation for OSM install command feat(metrics): Decouple Grafana and Prometheus installation for OSM install command Sep 8, 2020
snehachhabria
snehachhabria reviewed Sep 8, 2020
View reviewed changes
Copy link
Contributor

@snehachhabria snehachhabria left a comment

Choose a reason for hiding this comment

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

Please make the required documentation changes as well ( one such document is observability.md )

@snehachhabria snehachhabria self-requested a review September 8, 2020 22:09
@nshankar13 nshankar13 merged commit 15b35d1 into openservicemesh:main Sep 9, 2020
@ksubrmnn
Copy link
Contributor

Fixes #1599

@shashankram
Copy link
Member

Please make the required documentation changes as well ( one such document is observability.md )

@nshankar13 would you mind following this up with the requested documentation changes.

@snehachhabria snehachhabria mentioned this pull request Sep 10, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eduser25 eduser25 eduser25 left review comments

@ksubrmnn ksubrmnn ksubrmnn approved these changes

@snehachhabria snehachhabria Awaiting requested review from snehachhabria

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nshankar13 @shashankram @ksubrmnn @eduser25 @snehachhabria