*: Move collectors pkg to internal dir #632

lilic · 2019-01-15T17:58:12Z

Rename the existing functions that are used in collectors to collector
pkg.

What this PR does / why we need it:
Expose helper functions that can be used outside of kube-state-metrics and move collector package collectors to internal package. The internal directory is in the root of the project, this is so we can call this in the main.go file, import of a path containing the element “internal” is disallowed if the importing code is outside the tree rooted at the parent of the “internal” directory.

This is what we need from kube-state-metrics, but if @wanghaoran1988 needs something more, I can adjust things. Also open for naming discussions, etc. :)

cc @mxinden @brancz

Which issue(s) this PR fixes:
Fixes #579

brancz · 2019-01-15T18:14:31Z

This looks good to me (but giving @mxinden a chance for a review).

I think @mxinden was thinking through some changes to avoid duplication of metric name strings. It's unrelated to this change, just a heads up, that the API might change.

lilic · 2019-01-15T18:20:40Z

I think @mxinden was thinking through some changes to avoid duplication of metric name strings. It's unrelated to this change, just a heads up, that the API might change.

Yes, I was looking at that as well, sounds good improvement!

mxinden

Thanks for looking into this. Happy to do the rebase in case #633 merges first.

mxinden · 2019-01-16T12:22:02Z

pkg/collector/builder.go

@@ -0,0 +1,83 @@
+/*


Not sure builder.go is the right name here. Maybe metric_family.go instead as it is mostly concerned of groups of metrics?

Yes, you are right, should we move these to the metrics pkg instead to metric_family.go?

That is a great idea! Some of these could even be methods on the Family struct.

@mxinden which ones of the below were you thinking of binding to the Family struct? As all of them accept []metrics.FamilyGenerator and binding them to a single instance of the FamilyGenerator we would still need the functions that loop through the slices.

Also was thinking of moving it to pkg/metrics/family.go, sgty?

I was thinking of:

func (f *FamilyGenerator) GenerateHeader() string { header := strings.Builder{} header.WriteString("# HELP ") header.WriteString(f.Name) header.WriteByte(' ') header.WriteString(f.Help) header.WriteByte('\n') header.WriteString("# TYPE ") header.WriteString(f.Name) header.WriteByte(' ') header.WriteString(string(f.Type)) return header.String() }

Keeping the looping logic in ExtractMetricFamilyHeaders. But I can do that in a follow up.

Also was thinking of moving it to pkg/metrics/family.go, sgty?

👍

Keeping the looping logic in ExtractMetricFamilyHeaders.

Where would that live?

Maybe alongside the FamilyGenerator struct inside pkg/metrics/generator.go?

internal/collectors/builder.go

pkg/whiteblacklist/whiteblacklist.go

pkg/metrics/metrics.go

internal/collectors/builder.go

pkg/collector/collectors.go

pkg/metrics/metrics.go

mxinden · 2019-01-21T16:07:26Z

@lilic instead of cf42c78, what do you think about:

diff --git a/pkg/metrics_store/metrics_store_test.go b/pkg/metrics_store/metrics_store_test.go
index 6199d89..c750df7 100644
--- a/pkg/metrics_store/metrics_store_test.go
+++ b/pkg/metrics_store/metrics_store_test.go
@@ -9,9 +9,19 @@ import (
        "k8s.io/apimachinery/pkg/api/meta"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        "k8s.io/apimachinery/pkg/types"
-       "k8s.io/kube-state-metrics/pkg/metric"
 )
 
+// Mock metricFamily instead of importing /pkg/metric to prevent cyclic
+// dependency.
+type metricFamily struct {
+       value string
+}
+
+// Implement FamilyStringer interface.
+func (f *metricFamily) String() string {
+       return f.value
+}
+
 func TestObjectsSameNameDifferentNamespaces(t *testing.T) {
        serviceIDS := []string{"a", "b"}
 
@@ -21,15 +31,11 @@ func TestObjectsSameNameDifferentNamespaces(t *testing.T) {
                        t.Fatal(err)
                }
 
-               metric := metric.Metric{
-                       Name:        "kube_service_info",
-                       LabelKeys:   []string{"uid"},
-                       LabelValues: []string{string(o.GetUID())},
-                       Value:       1,
+               metricFamily := metricFamily{
+                       fmt.Sprintf("kube_service_info{uid=\"%v\"} 1", string(o.GetUID())),
                }
-               metricFamily := metric.Family{&metric}
 
-               return []FamilyStringer{metricFamily}
+               return []FamilyStringer{&metricFamily}
        }
 
        ms := NewMetricsStore([]string{"Information about service."}, genFunc)

This would only alter test code but keep interfaces close to the caller side.

lilic · 2019-01-22T09:54:25Z

@mxinden

+// Mock metricFamily instead of importing /pkg/metric to prevent cyclic
+// dependency.

I am curious where would this happen?

This would only alter test code but keep interfaces close to the caller side.

Makes sense!

mxinden · 2019-01-22T10:25:34Z

I am curious where would this happen?

I am a bit confused. type metricFamily struct { would be the mock, instead of importing metrics.Family. Does that explain the question?

In regards to the merge conflicts, those are due to #636. In case rebasing is not that easy I am happy to help out. Sorry for the inconvenience.

lilic · 2019-01-22T11:20:56Z

I am a bit confused. type metricFamily struct { would be the mock, instead of importing metrics.Family. Does that explain the question?

Sorry, I thought the initial comment was referring that can happen in my patch. All good 👍

lilic · 2019-01-22T16:44:04Z

In regards to the merge conflicts, those are due to #636. In case rebasing is not that easy I am happy to help out. Sorry for the inconvenience.

No worries! Wasn't too bad after all :)

lilic · 2019-01-22T17:37:05Z

@mxinden I addressed the comment above and rebased to current master, can you have another look, thanks!

mxinden

This looks great to me.

I don't know how to tell the Kubernetes bot to squash the commits before merging. Tide seems to have a squash option, but I don't know how to set that.

@lilic in case you are lost as I am, would you mind squashing the commits manually into one or two bigger commits?

This makes the rest of the packages useful to be used in a standalone library without importing the kube-state-metrics specific collectors. * Rename collectors -> collector package * Rename metrics -> metric package * Add metricFamily mocking in tests to prevent cyclic dependency.

lilic · 2019-01-23T09:56:00Z

@mxinden

@lilic in case you are lost as I am, would you mind squashing the commits manually into one or two bigger commits?

I squashed into one commit, but detailed the important commits in the commit msg. Hope that is okay?

mxinden · 2019-01-23T16:23:36Z

/approve

brancz · 2019-01-23T16:32:28Z

/approve doesn't include lgtm

/lgtm

k8s-ci-robot · 2019-01-23T16:32:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, LiliC, mxinden

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [brancz,mxinden]

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

ikaldus · 2019-03-01T08:51:51Z

Could it be related to this PR if unable to go get github.com/kubernetes/kube-state-metrics because of
src/github.com/kubernetes/kube-state-metrics/main.go:41:2: use of internal package k8s.io/kube-state-metrics/internal/collector not allowed?

lilic · 2019-03-01T09:26:57Z

@ikaldus You need to do go get k8s.io/kube-state-metrics, that worked for me.

ikaldus · 2019-03-05T08:35:01Z

I'm not sure, how I ended up with that github.com, but you are right... go get k8s.io/kube-state-metrics works flawlessly. Thanks for pointing out the obvious.

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 15, 2019

k8s-ci-robot requested review from brancz and zouyee January 15, 2019 17:58

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 15, 2019

mxinden reviewed Jan 16, 2019

View reviewed changes

wanghaoran1988 reviewed Jan 17, 2019

View reviewed changes

internal/collectors/builder.go Outdated Show resolved Hide resolved

wanghaoran1988 reviewed Jan 17, 2019

View reviewed changes

internal/collectors/builder.go Outdated Show resolved Hide resolved

mxinden reviewed Jan 18, 2019

View reviewed changes

pkg/collector/collectors.go Outdated Show resolved Hide resolved

mxinden reviewed Jan 18, 2019

View reviewed changes

pkg/metrics/metrics.go Outdated Show resolved Hide resolved

lilic force-pushed the lili/lib branch from d1319c4 to cf42c78 Compare January 21, 2019 15:57

lilic force-pushed the lili/lib branch from f95fa27 to 85d7109 Compare January 22, 2019 16:40

lilic force-pushed the lili/lib branch from 19ba515 to 2a2455c Compare January 22, 2019 17:23

mxinden approved these changes Jan 23, 2019

View reviewed changes

lilic force-pushed the lili/lib branch 3 times, most recently from fe41f0d to 4832763 Compare January 23, 2019 09:48

lilic force-pushed the lili/lib branch from 4832763 to 3498aa7 Compare January 23, 2019 09:51

k8s-ci-robot assigned brancz Jan 23, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019

k8s-ci-robot merged commit 58a8c11 into kubernetes:master Jan 23, 2019

lilic deleted the lili/lib branch January 23, 2019 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Move collectors pkg to internal dir #632

*: Move collectors pkg to internal dir #632

lilic commented Jan 15, 2019

brancz commented Jan 15, 2019

lilic commented Jan 15, 2019

mxinden left a comment

mxinden Jan 16, 2019

lilic Jan 16, 2019

mxinden Jan 17, 2019

lilic Jan 18, 2019

mxinden Jan 18, 2019

lilic Jan 18, 2019

mxinden Jan 18, 2019

mxinden commented Jan 21, 2019

lilic commented Jan 22, 2019 •

edited

Loading

mxinden commented Jan 22, 2019

lilic commented Jan 22, 2019

lilic commented Jan 22, 2019

lilic commented Jan 22, 2019

mxinden left a comment

lilic commented Jan 23, 2019

mxinden commented Jan 23, 2019

brancz commented Jan 23, 2019

k8s-ci-robot commented Jan 23, 2019

ikaldus commented Mar 1, 2019

lilic commented Mar 1, 2019

ikaldus commented Mar 5, 2019

*: Move collectors pkg to internal dir #632

*: Move collectors pkg to internal dir #632

Conversation

lilic commented Jan 15, 2019

brancz commented Jan 15, 2019

lilic commented Jan 15, 2019

mxinden left a comment

Choose a reason for hiding this comment

mxinden Jan 16, 2019

Choose a reason for hiding this comment

lilic Jan 16, 2019

Choose a reason for hiding this comment

mxinden Jan 17, 2019

Choose a reason for hiding this comment

lilic Jan 18, 2019

Choose a reason for hiding this comment

mxinden Jan 18, 2019

Choose a reason for hiding this comment

lilic Jan 18, 2019

Choose a reason for hiding this comment

mxinden Jan 18, 2019

Choose a reason for hiding this comment

mxinden commented Jan 21, 2019

lilic commented Jan 22, 2019 • edited Loading

mxinden commented Jan 22, 2019

lilic commented Jan 22, 2019

lilic commented Jan 22, 2019

lilic commented Jan 22, 2019

mxinden left a comment

Choose a reason for hiding this comment

lilic commented Jan 23, 2019

mxinden commented Jan 23, 2019

brancz commented Jan 23, 2019

k8s-ci-robot commented Jan 23, 2019

ikaldus commented Mar 1, 2019

lilic commented Mar 1, 2019

ikaldus commented Mar 5, 2019

lilic commented Jan 22, 2019 •

edited

Loading