-
Notifications
You must be signed in to change notification settings - Fork 630
Adding data plane metrics to ApiServerSource #1786
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
Changes from all commits
4589c3a
18e4586
418e3e5
0982be1
ec0bd10
311778a
6e85c31
416d5f1
8d3fd3b
b5c7652
502e355
3aeb383
32f723e
a089b2d
d044b0d
4951406
01ec663
07230eb
0e065b5
d37e614
180f5d6
2efd308
0c14a00
e6b8962
f73ddce
48fcc64
ab0d043
d18d1dd
b4c4059
0ebae80
9426433
6af5ef9
bc8e6e2
891a1f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,25 +18,31 @@ package main | |||||
|
|
||||||
| import ( | ||||||
| "flag" | ||||||
| "log" | ||||||
| "fmt" | ||||||
| "strings" | ||||||
|
|
||||||
| // Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters). | ||||||
| // Uncomment the following line to load the gcp plugin | ||||||
| // (only required to authenticate against GKE clusters). | ||||||
| // _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" | ||||||
|
|
||||||
| "github.com/kelseyhightower/envconfig" | ||||||
| "go.uber.org/zap" | ||||||
| "go.uber.org/zap/zapcore" | ||||||
| "k8s.io/apimachinery/pkg/api/meta" | ||||||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||||||
| "k8s.io/client-go/dynamic" | ||||||
| "k8s.io/client-go/tools/clientcmd" | ||||||
| "knative.dev/eventing/pkg/adapter/apiserver" | ||||||
| "knative.dev/eventing/pkg/kncloudevents" | ||||||
| "knative.dev/eventing/pkg/tracing" | ||||||
| "knative.dev/eventing/pkg/utils" | ||||||
| "knative.dev/pkg/logging" | ||||||
| "knative.dev/pkg/metrics" | ||||||
| "knative.dev/pkg/signals" | ||||||
| ) | ||||||
|
|
||||||
| const ( | ||||||
| component = "apiserversource" | ||||||
| ) | ||||||
|
|
||||||
| var ( | ||||||
| masterURL = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.") | ||||||
| kubeconfig = flag.String("kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.") | ||||||
|
|
@@ -60,52 +66,84 @@ type envConfig struct { | |||||
| Kind StringList `required:"true"` | ||||||
| Controller []bool `required:"true"` | ||||||
| LabelSelector StringList `envconfig:"SELECTOR" required:"true"` | ||||||
| Name string `envconfig:"NAME" required:"true"` | ||||||
| // MetricsConfigBase64 is a base64 encoded json string of | ||||||
| // metrics.ExporterOptions. This is used to configure the metrics exporter | ||||||
| // options, the config is stored in a config map inside the controllers | ||||||
| // namespace and copied here. | ||||||
| MetricsConfigBase64 string `envconfig:"K_METRICS_CONFIG" required:"true"` | ||||||
|
|
||||||
| // LoggingConfigBase64 is a base64 encoded json string of logging.Config. | ||||||
| // This is used to configure the logging config, the config is stored in | ||||||
| // a config map inside the controllers namespace and copied here. | ||||||
| LoggingConfigBase64 string `envconfig:"K_LOGGING_CONFIG" required:"true"` | ||||||
| } | ||||||
|
|
||||||
| // TODO: the controller should take the list of GVR | ||||||
|
|
||||||
| func main() { | ||||||
| flag.Parse() | ||||||
|
|
||||||
| logCfg := zap.NewProductionConfig() | ||||||
| logCfg.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder | ||||||
| dlogger, err := logCfg.Build() | ||||||
| var env envConfig | ||||||
| err := envconfig.Process("", &env) | ||||||
| if err != nil { | ||||||
| log.Fatalf("Error building logger: %v", err) | ||||||
| panic(fmt.Sprintf("Error processing env var: %s", err)) | ||||||
| } | ||||||
| // TODO move this util to pkg | ||||||
| // Convert base64 encoded json logging.Config to logging.Config. | ||||||
| loggingConfig, err := utils.Base64ToLoggingConfig(env.LoggingConfigBase64) | ||||||
| if err != nil { | ||||||
| fmt.Printf("[ERROR] failed to process logging config: %s", err.Error()) | ||||||
| // Use default logging config. | ||||||
| if loggingConfig, err = logging.NewConfigFromMap(map[string]string{}); err != nil { | ||||||
| // If this fails, there is no recovering. | ||||||
| panic(err) | ||||||
| } | ||||||
| } | ||||||
| loggerSugared, _ := logging.NewLoggerFromConfig(loggingConfig, component) | ||||||
| logger := loggerSugared.Desugar() | ||||||
| defer flush(loggerSugared) | ||||||
|
|
||||||
| // Convert base64 encoded json metrics.ExporterOptions to | ||||||
| // metrics.ExporterOptions. | ||||||
| metricsConfig, err := utils.Base64ToMetricsOptions( | ||||||
|
sayanh marked this conversation as resolved.
|
||||||
| env.MetricsConfigBase64) | ||||||
| if err != nil { | ||||||
| logger.Error("failed to process metrics options ", zap.Error(err)) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Don't need the extra space when using the desugared logger. Not here or the other lines. Once, again, I'll let it submit as-is, please send a follow up PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do, thanks @Harwayne! |
||||||
| } | ||||||
| logger := dlogger.Sugar() | ||||||
|
|
||||||
| var env envConfig | ||||||
| err = envconfig.Process("", &env) | ||||||
| if err := metrics.UpdateExporter(*metricsConfig, loggerSugared); err != nil { | ||||||
| logger.Error("failed to create the metrics exporter ", zap.Error(err)) | ||||||
| } | ||||||
|
|
||||||
| reporter, err := apiserver.NewStatsReporter() | ||||||
| if err != nil { | ||||||
| logger.Fatalw("Error processing environment", zap.Error(err)) | ||||||
| logger.Error("error building statsreporter", zap.Error(err)) | ||||||
| } | ||||||
|
|
||||||
| // set up signals so we handle the first shutdown signal gracefully | ||||||
| stopCh := signals.SetupSignalHandler() | ||||||
|
|
||||||
| cfg, err := clientcmd.BuildConfigFromFlags(*masterURL, *kubeconfig) | ||||||
| if err != nil { | ||||||
| logger.Fatalw("Error building kubeconfig", zap.Error(err)) | ||||||
| logger.Fatal("error building kubeconfig", zap.Error(err)) | ||||||
| } | ||||||
|
|
||||||
| logger = logger.With(zap.String("controller/apiserver", "adapter")) | ||||||
| logger.Info("Starting the controller") | ||||||
|
|
||||||
| client, err := dynamic.NewForConfig(cfg) | ||||||
| if err != nil { | ||||||
| logger.Fatalw("Error building dynamic client", zap.Error(err)) | ||||||
| logger.Fatal("error building dynamic client", zap.Error(err)) | ||||||
| } | ||||||
|
|
||||||
| if err = tracing.SetupStaticPublishing(logger, "apiserversource", tracing.OnePercentSampling); err != nil { | ||||||
| // If tracing doesn't work, we will log an error, but allow the importer to continue to | ||||||
| // start. | ||||||
| if err = tracing.SetupStaticPublishing(loggerSugared, "apiserversource", tracing.OnePercentSampling); err != nil { | ||||||
| // If tracing doesn't work, we will log an error, but allow the importer | ||||||
| // to continue to start. | ||||||
| logger.Error("Error setting up trace publishing", zap.Error(err)) | ||||||
| } | ||||||
|
|
||||||
| eventsClient, err := kncloudevents.NewDefaultClient(env.SinkURI) | ||||||
| if err != nil { | ||||||
| logger.Fatalw("Error building cloud event client", zap.Error(err)) | ||||||
| logger.Fatal("error building cloud event client", zap.Error(err)) | ||||||
| } | ||||||
|
|
||||||
| gvrcs := []apiserver.GVRC(nil) | ||||||
|
|
@@ -117,7 +155,7 @@ func main() { | |||||
|
|
||||||
| gv, err := schema.ParseGroupVersion(apiVersion) | ||||||
| if err != nil { | ||||||
| logger.Fatalw("Error parsing APIVersion", zap.Error(err)) | ||||||
| logger.Fatal("error parsing APIVersion", zap.Error(err)) | ||||||
| } | ||||||
| // TODO: pass down the resource and the kind so we do not have to guess. | ||||||
| gvr, _ := meta.UnsafeGuessKindToResource(schema.GroupVersionKind{Kind: kind, Group: gv.Group, Version: gv.Version}) | ||||||
|
|
@@ -134,9 +172,14 @@ func main() { | |||||
| GVRCs: gvrcs, | ||||||
| } | ||||||
|
|
||||||
| a := apiserver.NewAdaptor(cfg.Host, client, eventsClient, logger, opt) | ||||||
| logger.Info("starting kubernetes api adapter") | ||||||
| a := apiserver.NewAdaptor(cfg.Host, client, eventsClient, loggerSugared, opt, reporter, env.Name) | ||||||
| logger.Info("starting kubernetes api adapter.", zap.Any("adapter", env)) | ||||||
| if err := a.Start(stopCh); err != nil { | ||||||
| logger.Warn("start returned an error,", zap.Error(err)) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func flush(logger *zap.SugaredLogger) { | ||||||
| _ = logger.Sync() | ||||||
| metrics.FlushExporter() | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,12 @@ import ( | |
| rectesting "knative.dev/eventing/pkg/reconciler/testing" | ||
| ) | ||
|
|
||
| type mockReporter struct{} | ||
|
|
||
| func (r *mockReporter) ReportEventCount(args *ReportArgs, responseCode int) error { | ||
| return nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still prefer if we asserted the metrics were incremented in the test, but I won't hold up this PR for it. |
||
| } | ||
|
|
||
| func TestNewAdaptor(t *testing.T) { | ||
| ce := kncetesting.NewTestClient() | ||
| logger := zap.NewExample().Sugar() | ||
|
|
@@ -132,8 +138,8 @@ func TestNewAdaptor(t *testing.T) { | |
| } | ||
| for n, tc := range testCases { | ||
| t.Run(n, func(t *testing.T) { | ||
|
|
||
| a := NewAdaptor(tc.source, k8s, ce, logger, tc.opt) | ||
| r := &mockReporter{} | ||
| a := NewAdaptor(tc.source, k8s, ce, logger, tc.opt, r, "test-importer") | ||
|
|
||
| got, ok := a.(*adapter) | ||
| if !ok { | ||
|
|
@@ -171,8 +177,8 @@ func TestAdapter_StartRef(t *testing.T) { | |
| }, | ||
| }}, | ||
| } | ||
|
|
||
| a := NewAdaptor(source, k8s, ce, logger, opt) | ||
| r := &mockReporter{} | ||
| a := NewAdaptor(source, k8s, ce, logger, opt, r, "test-importer") | ||
|
|
||
| err := errors.New("test never ran") | ||
| stopCh := make(chan struct{}) | ||
|
|
@@ -206,8 +212,8 @@ func TestAdapter_StartResource(t *testing.T) { | |
| }, | ||
| }}, | ||
| } | ||
|
|
||
| a := NewAdaptor(source, k8s, ce, logger, opt) | ||
| r := &mockReporter{} | ||
| a := NewAdaptor(source, k8s, ce, logger, opt, r, "test-importer") | ||
|
|
||
| err := errors.New("test never ran") | ||
| stopCh := make(chan struct{}) | ||
|
|
@@ -292,22 +298,24 @@ func makeResourceAndTestingClient() (*resource, *kncetesting.TestCloudEventsClie | |
| ce := kncetesting.NewTestClient() | ||
| source := "unit-test" | ||
| logger := zap.NewExample().Sugar() | ||
|
|
||
| r := &mockReporter{} | ||
| return &resource{ | ||
| ce: ce, | ||
| source: source, | ||
| logger: logger, | ||
| ce: ce, | ||
| source: source, | ||
| logger: logger, | ||
| reporter: r, | ||
| }, ce | ||
| } | ||
|
|
||
| func makeRefAndTestingClient() (*ref, *kncetesting.TestCloudEventsClient) { | ||
| ce := kncetesting.NewTestClient() | ||
| source := "unit-test" | ||
| logger := zap.NewExample().Sugar() | ||
|
|
||
| r := &mockReporter{} | ||
| return &ref{ | ||
| ce: ce, | ||
| source: source, | ||
| logger: logger, | ||
| ce: ce, | ||
| source: source, | ||
| logger: logger, | ||
| reporter: r, | ||
| }, ce | ||
| } | ||
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.
Probably a foolish question, but why are we base64 encoding these? Flags tend to make sense, as it can be very annoying to get the escaping just right, but these are environment variables, where we don't have that problem.
Uh oh!
There was an error while loading. Please reload this page.
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.
My rationale was to be consistent with knative-gcp in here. @n3wscott What's your
take on this?