Configuration for tracing in sources #3026

slinkydeveloper · 2020-04-21T10:53:35Z

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Fixes #2978, depends on knative/pkg#1228

Proposed Changes

🧽 Now adapter.Main (we have 3 of these) has a new tracing config getter. Controllers of sources should populate the tracing environment variable with the contents of config-tracing config map. By default the tracing configuration is no-op.
Configured tracing via config map for ApiServerSource, follow-up PRs will fix it for other sources

Release Note

- Configuration of tracing for sources does not default anymore to the zipkin included in istio
- Configuration of tracing for sources now uses config-tracing config map

slinkydeveloper · 2020-04-21T10:54:45Z

/hold requires pkg update after this one is merged knative/pkg#1228

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper · 2020-04-27T07:52:20Z

/retest

slinkydeveloper · 2020-04-27T12:17:50Z

/retest

antoineco

A few nits.
My only real concerns are the obfuscated defaulting behind tracingconfig.JsonToTracingConfig, and the fact that we require yet another env var to be set (I'd like optional better).

pkg/adapter/v2/config.go

pkg/adapter/main.go

pkg/adapter/main_message_adapter.go

antoineco · 2020-04-28T08:29:49Z

pkg/adapter/v2/config.go

+func (e *EnvConfig) SetupTracing(logger *zap.SugaredLogger) error {
+	config, err := tracingconfig.JsonToTracingConfig(e.TracingConfigJson)
+	if err != nil {
+		logger.Error("Tracing configuration is invalid, using the no-op default", zap.Error(err))


I see this message, but I don't see config = defaultNoopTracingConfig. If this is hidden behind tracingconfig.JsonToTracingConfig, I believe the method should be renamed to reflect the intention, or the API should be improved to avoid obfuscating the defaulting.

For now it's godoc-ed that the method returns a default no-op configuration and I wanted to keep the consistency with the names of metrics and logging config. Since knative/pkg#1228 is already merged, if for you is fine, we can change this in a followup

Sure that's fine for me, just wanted to point it out.

pkg/adapter/v2/main.go

pkg/reconciler/apiserversource/apiserversource.go

pkg/reconciler/apiserversource/controller.go

pkg/reconciler/apiserversource/resources/receive_adapter.go

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper · 2020-04-29T10:30:52Z

the fact that we require yet another env var to be set (I'd like optional better).

Now it's optional and the default is a no-op valid configuration.

pkg/adapter/main.go

antoineco · 2020-04-29T10:59:33Z

/lgtm

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-metrics-robot · 2020-04-29T11:25:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/adapter/config.go	66.7%	71.4%	4.8
pkg/adapter/main.go	62.2%	64.6%	2.4
pkg/adapter/main_message_adapter.go	58.1%	60.9%	2.7
pkg/adapter/v2/config.go	65.0%	70.8%	5.8
pkg/reconciler/apiserversource/apiserversource.go	75.2%	74.8%	-0.4
pkg/reconciler/apiserversource/controller.go	93.3%	93.8%	0.4

slinkydeveloper · 2020-04-29T11:49:19Z

/retest

n3wscott · 2020-04-29T14:56:11Z

/lgtm
/approve

remove the hold if you are ready.

knative-prow-robot · 2020-04-29T14:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, slinkydeveloper

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

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

slinkydeveloper · 2020-04-29T15:00:17Z

/unhold

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 21, 2020

knative-prow-robot requested review from aslom and n3wscott April 21, 2020 10:53

slinkydeveloper mentioned this pull request Apr 21, 2020

CronJob event source attempts to invoke zipking when not needed #2643

Closed

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2020

slinkydeveloper added 2 commits April 27, 2020 08:24

Configuration for tracing in sources

6b940fc

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

tracing configuration in apiserversource

14c5d86

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper force-pushed the issues/2978 branch from dad2909 to 14c5d86 Compare April 27, 2020 06:24

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2020

antoineco reviewed Apr 28, 2020

View reviewed changes

Applied changes

e9ca605

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

antoineco reviewed Apr 29, 2020

View reviewed changes

pkg/adapter/main.go Outdated Show resolved Hide resolved

knative-prow-robot assigned antoineco Apr 29, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020

Applied changes

7d5683f

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020

knative-prow-robot assigned n3wscott Apr 29, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020

knative-prow-robot merged commit 6fd15c8 into knative:master Apr 29, 2020

slinkydeveloper deleted the issues/2978 branch April 29, 2020 15:01

lionelvillard mentioned this pull request Jan 22, 2021

Enable Tracing in Github source knative-extensions/eventing-github#103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration for tracing in sources #3026

Configuration for tracing in sources #3026

slinkydeveloper commented Apr 21, 2020 •

edited

Loading

slinkydeveloper commented Apr 21, 2020

slinkydeveloper commented Apr 27, 2020

slinkydeveloper commented Apr 27, 2020

antoineco left a comment

antoineco Apr 28, 2020 •

edited

Loading

slinkydeveloper Apr 29, 2020

antoineco Apr 29, 2020

slinkydeveloper commented Apr 29, 2020

antoineco commented Apr 29, 2020

knative-metrics-robot commented Apr 29, 2020

slinkydeveloper commented Apr 29, 2020

n3wscott commented Apr 29, 2020

knative-prow-robot commented Apr 29, 2020

slinkydeveloper commented Apr 29, 2020

Configuration for tracing in sources #3026

Configuration for tracing in sources #3026

Conversation

slinkydeveloper commented Apr 21, 2020 • edited Loading

Proposed Changes

slinkydeveloper commented Apr 21, 2020

slinkydeveloper commented Apr 27, 2020

slinkydeveloper commented Apr 27, 2020

antoineco left a comment

Choose a reason for hiding this comment

antoineco Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

slinkydeveloper Apr 29, 2020

Choose a reason for hiding this comment

antoineco Apr 29, 2020

Choose a reason for hiding this comment

slinkydeveloper commented Apr 29, 2020

antoineco commented Apr 29, 2020

knative-metrics-robot commented Apr 29, 2020

slinkydeveloper commented Apr 29, 2020

n3wscott commented Apr 29, 2020

knative-prow-robot commented Apr 29, 2020

slinkydeveloper commented Apr 29, 2020

slinkydeveloper commented Apr 21, 2020 •

edited

Loading

antoineco Apr 28, 2020 •

edited

Loading