Skip to content
This repository was archived by the owner on Jun 19, 2022. It is now read-only.

Wire in Logging and Metrics config maps to PullSubscription RA#219

Merged
knative-prow-robot merged 9 commits into
google:masterfrom
n3wscott:ra-metrics
Aug 29, 2019
Merged

Wire in Logging and Metrics config maps to PullSubscription RA#219
knative-prow-robot merged 9 commits into
google:masterfrom
n3wscott:ra-metrics

Conversation

@n3wscott
Copy link
Copy Markdown
Contributor

/assign @vaikas-google

@n3wscott
Copy link
Copy Markdown
Contributor Author

/retest

Comment thread pkg/pubsub/adapter/adapter.go Outdated
Comment thread pkg/pubsub/adapter/adapter.go Outdated
Comment thread pkg/pubsub/adapter/adapter.go Outdated
Comment thread pkg/reconciler/pullsubscription/resources/config.go
}

// Convert base64 encoded json logging.Config to logging.Config.
loggingConfig, err := resources.Base64ToLoggingConfig(startable.LoggingConfigBase64)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If somebody makes a logging error mistake, they will effectively kill all their pubsub messging. Maybe instead of panic log it with severity error. That way they can keep processing events, but without logging config changes taking effect?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I default if there is an error and if that still makes an error then I panic.

Comment thread cmd/pubsub/receive_adapter/main.go Outdated
// Convert base64 encoded json metrics.ExporterOptions to metrics.ExporterOptions.
metricsConfig, err := resources.Base64ToMetricsOptions(startable.MetricsConfigBase64)
if err != nil {
panic(err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I disable metrics if an error now. No panic.

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Aug 29, 2019

/lgtm
/approve

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/pullsubscription/controller.go 93.8% 94.4% 0.7
pkg/reconciler/pullsubscription/pullsubscription.go 66.9% 68.0% 1.1
pkg/reconciler/pullsubscription/resources/config.go Do not exist 72.9%

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, vaikas-google

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [n3wscott,vaikas-google]

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

@knative-prow-robot knative-prow-robot merged commit d72a338 into google:master Aug 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants