SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function #2566

mgencur · 2022-08-03T09:12:32Z

…acer

The caller can then call Shutdown() to properly flush buffered Spans
before shutdown

This is incompatible change that will require updates in many repositories. I can either prepare those PRs so that they can be merged when this change propagates to those repositories or I can keep the old implementation and also provide a new impl that returns the OpenCensusTracer. Whatever works best.
Initially, I will provide an example PR for knative/eventing that will show the required updates.

Changes

/kind enhancement

Fixes #2475

Release Note

Docs

knative-prow · 2022-08-03T09:12:33Z

@mgencur: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

…acer

The caller can then call Finish() to properly flush buffered Spans
before shutdown

This is incompatible change that will require updates in many repositories. I can either prepare those PRs so that they can be merged when this change propagates to those repositories or I can keep the old implementation and also provide a new impl that returns the OpenCensusTracer. Whatever works best.
Initially, I will provide an example PR for knative/eventing that will shows the required updates.

Changes

/kind

Fixes #

Release Note
Docs

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.

tracing/opencensus.go

mgencur · 2022-08-03T12:45:12Z

/retest-required

codecov · 2022-08-03T12:49:03Z

Codecov Report

Merging #2566 (2b37ebd) into main (7b8b060) will decrease coverage by 0.08%.
The diff coverage is 13.04%.

@@            Coverage Diff             @@
##             main    #2566      +/-   ##
==========================================
- Coverage   81.40%   81.31%   -0.09%     
==========================================
  Files         163      163              
  Lines        9740     9753      +13     
==========================================
+ Hits         7929     7931       +2     
- Misses       1574     1585      +11     
  Partials      237      237

Impacted Files	Coverage Δ
tracing/setup.go	`0.00% <0.00%> (ø)`
tracing/opencensus.go	`48.11% <19.35%> (-1.39%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dprotaso · 2022-08-03T14:33:32Z

We want to move away from OpenCensus - I don't think we should be returning the concrete type.

See: #2561 (comment)

mgencur · 2022-08-03T14:55:17Z

@dprotaso The downstream repositories are still using OpenCensus.
Looks like opentelemetry mostly uses something like this: Shutdown(ctx context.Context) error, E.g. here. Would it sound better to return a type that implements this function? Not sure what other options we have.
Something like this:

type Shutdowner interface {
	Shutdown(ctx context.Context) error
}

func SetupStaticPublishing(logger *zap.SugaredLogger, serviceName string, cfg *config.Config) (Shutdowner, error) {
	...
}

Anyways, we might end up with different needs. Its hard to predict future.
Edit: We might call the type "Tracer" with the option to add more functions later if needed:

type Tracer interface {
	Shutdown(ctx context.Context) error
}

mgencur · 2022-08-04T08:28:00Z

@dprotaso I've pushed a new commit and provided the Tracer type with the generic Shutdown function. WDYT?

daisy-ycguo · 2022-08-05T07:58:38Z

tracing/setup.go

 	}
-	return nil
+	return oct, nil


Is it possible to wrap the created object oct and hide the life cycle management in pkg/tracing other than returning it and delegating the management to other components?

I don't think so. Components that are creating the tracer will do some work and they need to shutdown the tracer at the end (before their own shutdown). And they do it by calling Shutdown throught the tracing.Tracer interface. So the components will now depend only on the pkg/tracing package.
If you have other thoughts how this could be done please let me know.
There's now knative/eventing#6474 which shows how this would be used.

tracing/setup.go

…acer * The caller can then call Finish() to properly flush buffered Spans before shutdown

Otherwise proceed futher and let globalOct.closer to be closed which actually flushes the remaining Spans.

dprotaso · 2022-08-05T15:19:16Z

tracing/opencensus.go

+		case <-ctx.Done():
+			return ctx.Err()


Is there any reason why we skip performing the shutdown when the context is cancelled?

It's a common way to allow for interrupting a long running functions through the context. OpenTelemetry uses this approach as well for interrupting the Shutdown function. I don't have a strong opionion about this - we could also keep continuing the shutdown until the end. The context would not be used in this implementation then.

tracing/opencensus.go

tracing/setup.go

daisy-ycguo · 2022-08-08T08:26:59Z

tracing/setup.go

 // just ensures that if generated, they are collected appropriately. This is normally done by using
 // tracing.HTTPSpanMiddleware as a middleware HTTP handler. The configuration will be dynamically
 // updated when the ConfigMap is updated.
-func SetupDynamicPublishing(logger *zap.SugaredLogger, configMapWatcher configmap.Watcher, serviceName, tracingConfigName string) error {
+func SetupPublishingWithDynamicConfig(logger *zap.SugaredLogger, configMapWatcher configmap.Watcher, serviceName, tracingConfigName string) (Tracer, error) {


Since we are changing the interface and we want to change all the invocation of SetupDynamicPublishing to SetupPublishingWithDynamicConfig, could we change to SetupPublishingWithDynamicConfigAndInitialValue directly? As I reported in issue #2557, SetupDynamicPublishing does not work properly since it doesn't call oct.ApplyConfig(cfg) at the first time.

If you agree, I think we don't need method SetupPublishingWithDynamicConfig because we will not use it any more. We just need method SetupPublishingWithDynamicConfigAndInitialValue.

@dprotaso your thoughts?

Since knative/eventing#6456 is not yet implemented and SetupDynamicPublishingWithInitialValue is not referenced yet I think it makes sense to do this:

Keep the current PR as it is and only have the latest/newest SetupPublishingWithDynamicConfigAndInitialValue

If the current PR is merged soon enough the new function can be used directly in a PR for knative/eventing

If there is a PR for knative/eventing merged before the current one I will put back the backwards-compatible function here so as to not break downstream

dprotaso · 2022-08-15T21:15:27Z

/lgtm
/approve

knative-prow · 2022-08-15T21:15:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, mgencur

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

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

dprotaso · 2022-08-15T21:18:36Z

FYI - knative.dev/pkg cut is tomorrow - if you plan on making any more edits tomorrow let me know and I can wait as long as it's straight forward

mgencur · 2022-08-16T04:58:13Z

@dprotaso @daisy-ycguo Thanks for reviews and merging!

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 3, 2022

mgencur commented Aug 3, 2022

View reviewed changes

tracing/opencensus.go Outdated Show resolved Hide resolved

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 3, 2022

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2022

mgencur force-pushed the opencensus_finish branch 2 times, most recently from 05c76b0 to 94a40f4 Compare August 4, 2022 11:01

mgencur changed the title ~~[WIP] SetupStaticPublishing and SetupDynamicPublishing returns OpenCensusTr…~~ [WIP] SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function Aug 5, 2022

mgencur mentioned this pull request Aug 5, 2022

Make sure tracer is shut down across components knative/eventing#6474

Merged

5 tasks

daisy-ycguo reviewed Aug 5, 2022

View reviewed changes

mgencur added 7 commits August 5, 2022 10:14

SetupStaticPublishing and SetupDynamicPublishing returns OpenCensusTr…

db7aad7

…acer * The caller can then call Finish() to properly flush buffered Spans before shutdown

Make sure the WithExporterFull checks if the config is nil

7df6de2

Parse cfg.Backend only if config is not nil

af76047

Otherwise proceed futher and let globalOct.closer to be closed which actually flushes the remaining Spans.

Provide Tracer type with Shutdown function

ab1cf24

Provide backwards compatible API

b3d6a50

Never return nil Tracer

c567279

Provide SetupPublishingWithDynamicConfigAndInitialValue

b6902ba

mgencur force-pushed the opencensus_finish branch from 94a40f4 to b6902ba Compare August 5, 2022 08:18

dprotaso reviewed Aug 5, 2022

View reviewed changes

Remove SetupDynamicPublishingWithInitialValue

2b37ebd

daisy-ycguo reviewed Aug 8, 2022

View reviewed changes

mgencur changed the title ~~[WIP] SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function~~ SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function Aug 15, 2022

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2022

knative-prow bot assigned dprotaso Aug 15, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2022

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2022

knative-prow bot merged commit 894c2f2 into knative:main Aug 15, 2022

This was referenced Aug 17, 2022

[main] Upgrade to latest dependencies knative/eventing#6485

Merged

Properly shutdown tracer to flush Spans knative-extensions/reconciler-test#376

Merged

Properly shutdown tracer when the work is done knative/serving#13226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function #2566

SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function #2566

mgencur commented Aug 3, 2022 •

edited

Loading

knative-prow bot commented Aug 3, 2022

Changes

mgencur commented Aug 3, 2022

codecov bot commented Aug 3, 2022 •

edited

Loading

dprotaso commented Aug 3, 2022

mgencur commented Aug 3, 2022 •

edited

Loading

mgencur commented Aug 4, 2022

daisy-ycguo Aug 5, 2022 •

edited

Loading

mgencur Aug 5, 2022 •

edited

Loading

dprotaso Aug 5, 2022

mgencur Aug 6, 2022 •

edited

Loading

daisy-ycguo Aug 8, 2022 •

edited

Loading

mgencur Aug 15, 2022

dprotaso commented Aug 15, 2022

knative-prow bot commented Aug 15, 2022

dprotaso commented Aug 15, 2022

mgencur commented Aug 16, 2022

SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function #2566

SetupStaticPublishing and SetupDynamicPublishing returns Tracer with Shutdown function #2566

Conversation

mgencur commented Aug 3, 2022 • edited Loading

Changes

knative-prow bot commented Aug 3, 2022

Changes

mgencur commented Aug 3, 2022

codecov bot commented Aug 3, 2022 • edited Loading

Codecov Report

dprotaso commented Aug 3, 2022

mgencur commented Aug 3, 2022 • edited Loading

mgencur commented Aug 4, 2022

daisy-ycguo Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

mgencur Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

dprotaso Aug 5, 2022

Choose a reason for hiding this comment

mgencur Aug 6, 2022 • edited Loading

Choose a reason for hiding this comment

daisy-ycguo Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

mgencur Aug 15, 2022

Choose a reason for hiding this comment

dprotaso commented Aug 15, 2022

knative-prow bot commented Aug 15, 2022

dprotaso commented Aug 15, 2022

mgencur commented Aug 16, 2022

mgencur commented Aug 3, 2022 •

edited

Loading

codecov bot commented Aug 3, 2022 •

edited

Loading

mgencur commented Aug 3, 2022 •

edited

Loading

daisy-ycguo Aug 5, 2022 •

edited

Loading

mgencur Aug 5, 2022 •

edited

Loading

mgencur Aug 6, 2022 •

edited

Loading

daisy-ycguo Aug 8, 2022 •

edited

Loading