Initial implementation of a Prometheus source #663

syedriko · 2019-10-21T20:00:28Z

This is a barebones implementation of a Prometheus source for Knative Eventing.
It addresses #612.

knative-prow-robot · 2019-10-21T20:00:43Z

Hi @syedriko. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

syedriko · 2019-10-21T20:06:18Z

/cc @matzew
/cc @lionelvillard

lionelvillard · 2019-10-21T21:28:22Z

/ok-to-test

lionelvillard · 2019-10-21T21:31:06Z

prometheus/pkg/apis/sources/v1alpha1/prometheussource_types.go

+	// ServiceAccountName holds the name of the Kubernetes service account
+	// as which the underlying K8s resources should be run. If unspecified
+	// this will default to the "default" service account for the namespace
+	// in which the CouchDbSource exists.


nit: PrometheusSource

lionelvillard · 2019-10-21T21:31:48Z

prometheus/pkg/reconciler/prometheussource.go

+	}
+)
+
+// Reconciler reconciles a CouchDbSource object


nit: PrometheusSource

lionelvillard · 2019-10-21T21:32:04Z

prometheus/pkg/reconciler/prometheussource.go

+	eventTypes := make([]eventingv1alpha1.EventType, 0)
+
+	// Only create EventTypes for Broker sinks.
+	// We add this check here in case the CouchDbSource was changed from Broker to non-Broker sink.


same (CouchDb->Prometheus)

lionelvillard · 2019-10-21T21:39:39Z

prometheus/pkg/adapter/adapter.go

+func (a *prometheusAdapter) makeEvent(payload interface{}) (*cloudevents.Event, error) {
+	event := cloudevents.NewEvent()
+	event.SetSource(a.source)
+	event.SetID(`prometheus_source_adapter_id`)


Source + ID must be unique.

What about documenting it by adding the CloudEvents adapter spec for Prometheus? (see https://github.com/cloudevents/spec/blob/master/adapters/github.md for an example)

(I'm doing the same for CouchDB)

lionelvillard · 2019-10-21T21:41:50Z

prometheus/pkg/adapter/adapter_test.go

+func newAdapterTestClient() *adapterTestClient {
+	return &adapterTestClient{
+		kncetesting.NewTestClient(),
+		make(chan struct{}),


you might need a buffered channel size 1, as the read/write is done in the same goroutine.

lionelvillard · 2019-10-21T21:46:44Z

prometheus/pkg/apis/sources/v1alpha1/register.go

+	// SchemeBuilder is used to add go types to the GroupVersionKind scheme
+	SchemeBuilder = &scheme.Builder{GroupVersion: SchemeGroupVersion}
+
+	AddToScheme = SchemeBuilder.AddToScheme


need to add the knownTypes. See https://github.com/knative/eventing-contrib/pull/661/files#diff-72f3ea7d0a1d6e4cb62509e5f562de91R48

lionelvillard · 2019-10-21T21:53:54Z

prometheus/pkg/reconciler/prometheussource.go

+	return nil
+}
+
+func (r *Reconciler) getReceiveAdapterImage() string {


Heads-up: when this PR gets merged we should do the same, which is to set the adapter image in controller

Sure thing.

@syedriko @lionelvillard it got merged!

lionelvillard · 2019-10-21T21:55:49Z

prometheus/pkg/reconciler/resources/receive_adapter.go

+		Name:  "PROMETHEUS_PROM_QL",
+		Value: spec.PromQL,
+	}, {
+		Name: "SYSTEM_NAMESPACE",


should be NAMESPACE

lionelvillard · 2019-10-21T21:57:37Z

prometheus/pkg/reconciler/resources/receive_adapter.go

+			FieldRef: &corev1.ObjectFieldSelector{
+				FieldPath: "metadata.namespace",
+			},
+		},


METRICS_DOMAIN, K_METRICS_CONFIG and K_LOGGING_CONFIG are missing. Can added after this PR is merged.

@syedriko just seeing this, but I added the required bits, se:

syedriko#1

Thanks, @matzew, I merged your branch.

matzew · 2019-10-22T09:51:14Z

prometheus/pkg/adapter/adapter.go

+}
+
+func (a *prometheusAdapter) makeEvent(payload interface{}) (*cloudevents.Event, error) {
+	event := cloudevents.NewEvent()


cloudevents.VersionV03 as argument ?

matzew · 2019-10-22T10:23:34Z

prometheus/README.md

+   kind: PrometheusSource
+   metadata:
+     name: prometheus-source
+     namespace: knative-sources


let's remove this ns, so (like other samples), it goes to default .

matzew · 2019-10-22T10:23:46Z

prometheus/demo/sink.yaml

+kind: Service
+metadata:
+  name: event-display
+  namespace: knative-sources


matzew · 2019-10-22T10:23:54Z

prometheus/demo/source.yaml

+kind: PrometheusSource
+metadata:
+  name: prometheus-source
+  namespace: knative-sources


matzew · 2019-10-22T10:24:02Z

prometheus/demo/source.yaml

@@ -0,0 +1,12 @@
+apiVersion: sources.eventing.knative.dev/v1alpha1
+kind: PrometheusSource


I get:

2019/10/22 10:22:03 Error processing env var: required key K_METRICS_CONFIG missing value

matzew · 2019-10-22T12:29:24Z

Got it running:

☁️  cloudevents.Event
Validation: valid
Context Attributes,
  specversion: 0.2
  type: dev.knative.prometheus.promql
  source: prometheus-source
  id: 3b0f4924-0d1c-4a3b-b15a-566c0ca67c83
  time: 2019-10-22T12:28:35.239998202Z
  contenttype: application/json
Data,
  {
    "status": "success",
    "data": {
      "resultType": "vector",
      "result": [
        {
          "metric": {
            "__name__": "ALERTS",
            "alertname": "ExampleAlertAlwaysFiring",
            "alertstate": "firing",
            "job": "alertmanager"
          },
          "value": [
            1571747315.23,
            "1"
          ]
        },
        {
          "metric": {
            "__name__": "ALERTS",
            "alertname": "ExampleAlertAlwaysFiring",
            "alertstate": "firing",
            "job": "node"
          },
          "value": [
            1571747315.23,
            "1"
          ]
        },
        {
          "metric": {
            "__name__": "ALERTS",
            "alertname": "ExampleAlertAlwaysFiring",
            "alertstate": "firing",
            "job": "prometheus"
          },
          "value": [
            1571747315.23,
            "1"
          ]
        },
        {
          "metric": {
            "__name__": "ALERTS",
            "alertname": "ExampleAlertAlwaysFiring",
            "alertstate": "firing",
            "job": "pushgateway"
          },
          "value": [
            1571747315.23,
            "1"
          ]
        }
      ]
    }
  }

as said in the comments, other sources are on CE 0.3 - let's update that

matzew · 2019-10-22T12:37:38Z

prometheus/pkg/adapter/adapter.go

+
+func (a *prometheusAdapter) makeEvent(payload interface{}) (*cloudevents.Event, error) {
+	event := cloudevents.NewEvent()
+	event.SetSource(a.source)


I think we should use something better here.

for instance in the kafka source, we add some context to it, like:

// KafkaEventSource returns the Kafka CloudEvent source. func KafkaEventSource(namespace, kafkaSourceName, topic string) string { return fmt.Sprintf("/apis/v1/namespaces/%s/kafkasources/%s#%s", namespace, kafkaSourceName, topic) }

were that is used as:

... event.SetSource(sourcesv1alpha1.KafkaEventSource(a.Namespace, a.Name, msg.Topic)) ...

vaikas

Sweet! Couple of tweaks wrt the api definition.

vaikas · 2019-10-22T16:12:26Z

prometheus/config/300-prometheussource.yaml

+            serverURL:
+              type: string
+            promQL:
+              type: string


Could we add "description" fields here as per:
https://github.com/knative/eventing/blob/master/docs/spec/sources.md

We sure can

vaikas · 2019-10-22T16:25:22Z

prometheus/pkg/apis/sources/v1alpha1/prometheussource_types.go

+	// Sink is a reference to an object that will resolve to a domain
+	// name to use as the sink.
+	// +optional
+	Sink *corev1.ObjectReference `json:"sink,omitempty"`


This should be a Destination:
https://github.com/knative/eventing/blob/master/docs/spec/sources.md

And we should also have CloudEventOverrides here as well.

I'd like to push back on this if that's ok. Can we merge this PR as it stands wrt sinks and ceOverrides and do the conversion of sinks to Destinations across the board in all the sources?
It would be nice to update https://github.com/knative/eventing/blob/master/pkg/duck/sinks.go to retrieve URIs from Destinations according to https://github.com/knative/eventing/blob/master/docs/spec/sources.md, update vendored knative.dev/pkg and knative.dev/eventing in eventing-contrib.

I agree w/ @syedriko on this subject

Keeping it consistent and doing conversion and testing (and upgrade docs if needed) for all sources in contrib sounds good to me

vaikas · 2019-10-22T16:26:15Z

prometheus/pkg/apis/sources/v1alpha1/prometheussource_types.go

+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"knative.dev/pkg/apis/duck"
+	duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"


Might as well use duck v1 instead of the v1beta1?

We sure might

vaikas · 2019-10-22T16:26:59Z

prometheus/pkg/apis/sources/v1alpha1/prometheussource_types.go

+
+// PrometheusSourceStatus defines the observed state of PrometheusSource
+type PrometheusSourceStatus struct {
+	// inherits duck/v1alpha1 Status, which currently provides:


v1alpha1 => v1

vaikas · 2019-10-22T16:28:30Z

prometheus/pkg/apis/sources/v1alpha1/prometheussource_types.go

+// Check that Prometheus source can be validated and can be defaulted.
+var _ runtime.Object = (*PrometheusSource)(nil)
+
+// Check that we can create OwnerReferences to a Configuration.


s/Configuration/Prometheus Source/?

lionelvillard · 2019-10-23T13:17:39Z

prometheus/pkg/adapter/adapter.go

+}
+
+func (a *prometheusAdapter) Start(stopCh <-chan struct{}) error {
+	return wait.PollUntil(5*time.Second, a.send, stopCh)


I think wait.Until is better here. PollUntil stops when there is an error, meaning that the adapter will stop running when an event couldn't be sent, which is not what we want. Instead we should log the error (for now) and keep going.

I was wondering about that, too, tbh. And the hardcoded interval here.

In that vein, https://github.com/knative/eventing/blob/ec4dcbbd5c35079dcbab80a94d22000338dda55b/pkg/adapter/main.go#L43 shouldn't return anything.

Maybe. I'm not so sure that all errors can be caught at initialization time. But this is something to consider.

matzew · 2019-10-23T13:19:02Z

/lgtm

Leaving the final approve for @vaikas-google

lionelvillard · 2019-10-23T13:23:37Z

prometheus/pkg/reconciler/prometheussource.go

+	*reconciler.Base
+
+	receiveAdapterImage string
+	once                sync.Once


evidently not

matzew · 2019-10-24T14:31:51Z

/lgtm
/approve

knative-prow-robot · 2019-10-24T14:32:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, syedriko

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~~ [matzew]

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

syedriko · 2019-10-24T14:50:13Z

Squashed the commits.

Can I be accepted into the knative org or should I remove myself from OWNERS_ALIASES?

The question went to knative-admins@googlegroups.com.

knative-metrics-robot · 2019-10-24T14:50:59Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
prometheus/pkg/adapter/adapter.go	Do not exist	70.0%
prometheus/pkg/apis/sources/v1alpha1/prometheus_lifecycle.go	Do not exist	53.8%
prometheus/pkg/apis/sources/v1alpha1/prometheussource_types.go	Do not exist	100.0%
prometheus/pkg/apis/sources/v1alpha1/register.go	Do not exist	100.0%

syedriko · 2019-10-24T21:39:28Z

/verify-owners

matzew · 2019-10-25T08:05:36Z

/lgtm

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 21, 2019

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2019

knative-prow-robot added the area/test-and-release label Oct 21, 2019

knative-prow-robot requested review from chaodaiG and n3wscott October 21, 2019 20:01

knative-prow-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 21, 2019

knative-prow-robot requested review from lionelvillard and matzew October 21, 2019 20:06

syedriko mentioned this pull request Oct 21, 2019

Implement an Eventing source for Prometheus #612

Closed

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2019

lionelvillard reviewed Oct 21, 2019

View reviewed changes

syedriko force-pushed the prometheus_source branch 6 times, most recently from fc19213 to d166dd7 Compare October 22, 2019 03:16

matzew reviewed Oct 22, 2019

View reviewed changes

vaikas reviewed Oct 22, 2019

View reviewed changes

syedriko force-pushed the prometheus_source branch from d166dd7 to b0eaa57 Compare October 23, 2019 04:19

lionelvillard reviewed Oct 23, 2019

View reviewed changes

knative-prow-robot assigned matzew Oct 23, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2019

lionelvillard reviewed Oct 23, 2019

View reviewed changes

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2019

Initial implementation of a Prometheus source

ff9cd64

syedriko force-pushed the prometheus_source branch from 3477a8e to ff9cd64 Compare October 24, 2019 14:47

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2019

knative-prow-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 24, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2019

knative-prow-robot merged commit d9b9399 into knative:master Oct 25, 2019

syedriko deleted the prometheus_source branch October 25, 2019 15:43

		@@ -0,0 +1,12 @@
		apiVersion: sources.eventing.knative.dev/v1alpha1
		kind: PrometheusSource

Initial implementation of a Prometheus source #663

Initial implementation of a Prometheus source #663

Conversation

syedriko commented Oct 21, 2019 • edited Loading

knative-prow-robot commented Oct 21, 2019

syedriko commented Oct 21, 2019

lionelvillard commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionelvillard Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Oct 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Oct 24, 2019

knative-prow-robot commented Oct 24, 2019

syedriko commented Oct 24, 2019 • edited Loading

knative-metrics-robot commented Oct 24, 2019

syedriko commented Oct 24, 2019

matzew commented Oct 25, 2019

syedriko commented Oct 21, 2019 •

edited

Loading

lionelvillard Oct 21, 2019 •

edited

Loading

syedriko commented Oct 24, 2019 •

edited

Loading