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/expose metrics only specific ns #94

Conversation

kimiyasharifi
Copy link

@kimiyasharifi kimiyasharifi commented Jun 5, 2024

This pull request introduces changes to enable the exporter to expose metrics only for ns labeled with specific label. Introduced a new env var EPHEMERAL_STORAGE_LABEL to specify the label selector for ns to monitor.

  • getMonitoredNamespaces(clientset *kubernetes.Clientset) ([]string, error) now reads the label selector from the EPHEMERAL_STORAGE_LABEL env var.
  • setMetrics(clientset *kubernetes.Clientset, nodeName string, monitoredNamespaces map[string]bool) now receives a map of monitored ns to check if a pod’s ns is in the monitored list.

Additionally, this pull request adds the capability to expose metrics for all namespaces by default, unless a specific environment variable, ENABLE_SPECIFIC_NAMESPACE_MONITORING, is set to true. This logic is implemented in the getMonitoredNamespaces function.

@jmcgrath207
Copy link
Owner

Hi @kimiyasharifi, thanks for you pull request. Overall I am for this feature, but we do need to revert of some changes. I will annotate those now.

I also enabled the e2e test on this PR incase you can't run the test local. Currently it's failing due to the

https://github.com/jmcgrath207/k8s-ephemeral-storage-metrics/actions/runs/9428091753/job/25976673651?pr=94#step:6:8

Copy link
Owner

@jmcgrath207 jmcgrath207 left a comment

Choose a reason for hiding this comment

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

Revert all of charts to chart
Do not bump the version.

Copy link
Owner

Choose a reason for hiding this comment

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

Revert this.

select {
case <-ticker.C:
Copy link
Owner

Choose a reason for hiding this comment

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

What is the benefit of doing it this way?

@@ -139,12 +139,28 @@ func (n *Node) Watch() {
// Start the informer to begin watching for Node events
go sharedInformerFactory.Start(stopCh)

// Use a ticker for periodic actions
ticker := time.NewTicker(time.Duration(n.sampleInterval) * time.Second)
Copy link
Owner

Choose a reason for hiding this comment

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

Why should we do it this way?

@@ -23,6 +27,35 @@ var (
Pod pod.Collector
)

func getMonitoredNamespaces(clientset *kubernetes.Clientset) ([]string, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Close. This will get our namespaces on init only. We will want to update our cache of namespaces periodically incase need ones were added.

A good example would be this node watch with a shard informer.
https://github.com/jmcgrath207/k8s-ephemeral-storage-metrics/blob/master/pkg/node/k8s.go#L106

Wouldn't be against following the same pattern and putting that code under pkg/k8s.go

@jmcgrath207
Copy link
Owner

Hey not see any activity. Closing feel free to reopen once changes are completed.

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.

2 participants