-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Conversation
…, fix import error in utility package
…error in httputil
…k discovery package
config/config.go
Outdated
} | ||
return checkOverflow(c.XXX, "TLS config") | ||
} | ||
|
||
// ServiceDiscoveryConfig configures lists of different service discovery mechanisms. | ||
type ServiceDiscoveryConfig struct { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about discovery/config
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…ge instead of config.TargetGroup
pkg/targetgroup/targetgroup.go
Outdated
// limitations under the License. | ||
|
||
package targetgroup | ||
|
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, agreed.
There was a problem hiding this comment.
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.
rather than moving configs in common/pkg please try to move configs as close as possible to each package that is using it. 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.
I little more verbose , but will be better for decoupling |
discovery/consul/consul.go
Outdated
@@ -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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found it: microsoft/vscode-go#613 (comment)
discovery/kubernetes/kubernetes.go
Outdated
KubernetesRolePod KubernetesRole = "pod" | ||
KubernetesRoleService KubernetesRole = "service" | ||
KubernetesRoleEndpoint KubernetesRole = "endpoints" | ||
KubernetesRoleIngress KubernetesRole = "ingress" |
There was a problem hiding this comment.
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.
discovery/kubernetes/kubernetes.go
Outdated
|
||
// KubernetesNamespaceDiscovery is the configuration for discovering | ||
// Kubernetes namespaces. | ||
type KubernetesNamespaceDiscovery struct { |
There was a problem hiding this comment.
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).
discovery/openstack/openstack.go
Outdated
} | ||
|
||
// OpenStackRole is role of the target in OpenStack. | ||
type OpenStackRole string |
There was a problem hiding this comment.
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...
cmd/promtool/main.go
Outdated
@@ -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" |
There was a problem hiding this comment.
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.
👍 Super cool besides the (mostly minor) comments! Thanks! |
BearerTokenFile: filepath.FromSlash("testdata/valid_token_file"), | ||
}, | ||
|
||
ServiceDiscoveryConfig: ServiceDiscoveryConfig{ | ||
StaticConfigs: []*TargetGroup{ | ||
ServiceDiscoveryConfig: sd_config.ServiceDiscoveryConfig{ |
There was a problem hiding this comment.
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.
discovery/kubernetes/kubernetes.go
Outdated
KubernetesRolePod Role = "pod" | ||
KubernetesRoleService Role = "service" | ||
KubernetesRoleEndpoint Role = "endpoints" | ||
KubernetesRoleIngress Role = "ingress" |
There was a problem hiding this comment.
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).
This is 👍 from my side now. Followup items are:
|
Will merge in a couple of hours unless there's final objections. |
) * 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
Made a separate package for
targetGroup
, moved common stuff used across SD intoutil/config
Fixes #3013