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

Refactor SD configuration to remove config dependency #3629

Merged
merged 30 commits into from
Dec 29, 2017
Merged

Refactor SD configuration to remove config dependency #3629

merged 30 commits into from
Dec 29, 2017

Conversation

shubheksha
Copy link
Contributor

Made a separate package for targetGroup, moved common stuff used across SD into util/config
Fixes #3013

config/config.go Outdated
}
return checkOverflow(c.XXX, "TLS config")
}

// ServiceDiscoveryConfig configures lists of different service discovery mechanisms.
type ServiceDiscoveryConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this struct should move into SD too

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this one could go into the main discovery package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can't go into manager.go in the discovery package because it'll create an import cycle.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about discovery/config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That worked, thank you!

config/config.go Outdated
"github.com/prometheus/common/model"
"gopkg.in/yaml.v2"
"github.com/prometheus/prometheus/pkg/targetgroup"
configUtil "github.com/prometheus/prometheus/util/config"
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is needed across the project, I believe it should go in the common repository.

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree... but IIRC @fabxc dislikes putting too much into common and prefers duplication? Would be good to get his opinion too.

Copy link
Contributor

Choose a reason for hiding this comment

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

This library includes security-sensitive code, that's not something we should be duplicating. Duplication also leads to drift.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me. But how about still keeping the code in this repo for this PR and doing the splitout separately. Keeps this PR simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. This PR is going to be intricate enough as-is.

// limitations under the License.

package targetgroup

Copy link
Contributor

Choose a reason for hiding this comment

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

why not move this under retrieval/config ?

Copy link
Member

Choose a reason for hiding this comment

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

It's a shared concept between SD and retrieval, so it was kind of unclear where to put it. A shared place that isn't tied to either seemed to make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel SD is where is really belongs. It's the output of SD.

Copy link
Member

Choose a reason for hiding this comment

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

discovery or discovery/config then? The latter would make it more decoupled, but config is only part of what target groups are used for (the output of SDs and input to retrieval being the other use).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say discovery. We should probably also split out the two different uses of this struct (SD output vs. static_config input) into separate structs as the struct doesn't match the JSON. Probably not for this PR though.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can't go in either of those because it'll create import cycles.
For discovery, config imports manager.go already imports config
For discovery/config, it creates import cycles with all other SD discovery packages as discovery/config imports all of them.

@krasi-georgiev
Copy link
Contributor

krasi-georgiev commented Dec 28, 2017

rather than moving configs in common/pkg please try to move configs as close as possible to each package that is using it.
retrieval/config
discovery/config

I am working on another PR for more decoupling and this will help a lot.

....

when both packages share a config I think best to pass a struct to the package and inside assign it to the corresponding config.
for example:

package.New(cfg struct{}){
    config:=package.Config{
        Field1: cfg.Field1
        ....
    }
}

I little more verbose , but will be better for decoupling

@@ -25,12 +25,14 @@ import (
"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
consul "github.com/hashicorp/consul/api"
"github.com/mwitkow/go-conntrack"
conntrack "github.com/mwitkow/go-conntrack"
Copy link
Member

Choose a reason for hiding this comment

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

Is this (and same in some other places) because your editor still has trouble with package names that are different from the last directory name component? It would be good to get to the bottom of that issue in general, as those aliases shouldn't be needed... then again, I'm not really opposed to them either, as it could make it easier for humans to see what is what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KubernetesRolePod KubernetesRole = "pod"
KubernetesRoleService KubernetesRole = "service"
KubernetesRoleEndpoint KubernetesRole = "endpoints"
KubernetesRoleIngress KubernetesRole = "ingress"
Copy link
Member

Choose a reason for hiding this comment

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

I think all these can lose their Kubernetes prefix, since they are inside the kubernetes package now and the names are sufficiently clear without the prefix.


// KubernetesNamespaceDiscovery is the configuration for discovering
// Kubernetes namespaces.
type KubernetesNamespaceDiscovery struct {
Copy link
Member

Choose a reason for hiding this comment

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

Same here (remove the Kubernetes prefix).

}

// OpenStackRole is role of the target in OpenStack.
type OpenStackRole string
Copy link
Member

Choose a reason for hiding this comment

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

This and the below consts can also lose their prefixes now...

@@ -28,6 +28,7 @@ import (
"github.com/prometheus/prometheus/config"
"github.com/prometheus/prometheus/pkg/rulefmt"
"github.com/prometheus/prometheus/promql"
configUtil "github.com/prometheus/prometheus/util/config"
Copy link
Member

Choose a reason for hiding this comment

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

Though it looks a bit un-Go-ey, we've always snake_cased multi-word package names, so it'd be good to stay consistent with that. I.e. configUtil -> config_util and yamlUtil -> yaml_util, etc.

@juliusv
Copy link
Member

juliusv commented Dec 28, 2017

👍 Super cool besides the (mostly minor) comments! Thanks!

BearerTokenFile: filepath.FromSlash("testdata/valid_token_file"),
},

ServiceDiscoveryConfig: ServiceDiscoveryConfig{
StaticConfigs: []*TargetGroup{
ServiceDiscoveryConfig: sd_config.ServiceDiscoveryConfig{
Copy link
Member

Choose a reason for hiding this comment

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

sd_config.ServiceDiscoveryConfig stutters now, but I think that's ok to fix in a later change that removes config deps from the main discovery package and thus allows us to just move the main SD config there.

KubernetesRolePod Role = "pod"
KubernetesRoleService Role = "service"
KubernetesRoleEndpoint Role = "endpoints"
KubernetesRoleIngress Role = "ingress"
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I meant removing the Kubernetes prefix in the constant names as well (not just the type name).

@juliusv
Copy link
Member

juliusv commented Dec 29, 2017

This is 👍 from my side now.

Followup items are:

  • split up TargetGroup into two structs: one that resembles the YAML config that is read, and one that is used for internal interchange between SD and retrieval.
  • remove the discovery -> config dependency by having a better decoupled config reloading mechanism. That and the above then also means that we need fewer subpackages under discovery.
  • move Secret and some other shared config types to the common repo.

@juliusv
Copy link
Member

juliusv commented Dec 29, 2017

Will merge in a couple of hours unless there's final objections.

@juliusv juliusv merged commit ec94df4 into prometheus:master Dec 29, 2017
krasi-georgiev pushed a commit to krasi-georgiev/prometheus that referenced this pull request Jan 10, 2018
)

* refactor: move targetGroup struct and CheckOverflow() to their own package

* refactor: move auth and security related structs to a utility package, fix import error in utility package

* refactor: Azure SD, remove SD struct from config

* refactor: DNS SD, remove SD struct from config into dns package

* refactor: ec2 SD, move SD struct from config into the ec2 package

* refactor: file SD, move SD struct from config to file discovery package

* refactor: gce, move SD struct from config to gce discovery package

* refactor: move HTTPClientConfig and URL into util/config, fix import error in httputil

* refactor: consul, move SD struct from config into consul discovery package

* refactor: marathon, move SD struct from config into marathon discovery package

* refactor: triton, move SD struct from config to triton discovery package, fix test

* refactor: zookeeper, move SD structs from config to zookeeper discovery package

* refactor: openstack, remove SD struct from config, move into openstack discovery package

* refactor: kubernetes, move SD struct from config into kubernetes discovery package

* refactor: notifier, use targetgroup package instead of config

* refactor: tests for file, marathon, triton SD - use targetgroup package instead of config.TargetGroup

* refactor: retrieval, use targetgroup package instead of config.TargetGroup

* refactor: storage, use config util package

* refactor: discovery manager, use targetgroup package instead of config.TargetGroup

* refactor: use HTTPClient and TLS config from configUtil instead of config

* refactor: tests, use targetgroup package instead of config.TargetGroup

* refactor: fix tagetgroup.Group pointers that were removed by mistake

* refactor: openstack, kubernetes: drop prefixes

* refactor: remove import aliases forced due to vscode bug

* refactor: move main SD struct out of config into discovery/config

* refactor: rename configUtil to config_util

* refactor: rename yamlUtil to yaml_config

* refactor: kubernetes, remove prefixes

* refactor: move the TargetGroup package to discovery/

* refactor: fix order of imports
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.

None yet

4 participants