[injection/sharedmain] OTel Support #3190

dprotaso · 2025-06-25T13:11:34Z

This stacks onto #3188

codecov · 2025-06-25T15:14:27Z

Codecov Report

Attention: Patch coverage is 62.96296% with 10 lines in your changes missing coverage. Please review.

Project coverage is 75.91%. Comparing base (5e2512c) to head (db62f56).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
observability/resource/default.go	77.27%	3 Missing and 2 partials ⚠️
system/env.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3190      +/-   ##
==========================================
- Coverage   75.96%   75.91%   -0.05%     
==========================================
  Files         205      205              
  Lines       11690    11709      +19     
==========================================
+ Hits         8880     8889       +9     
- Misses       2539     2547       +8     
- Partials      271      273       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

evankanderson

A few comments about usage, but this makes sense to me overall.

injection/sharedmain/main.go

evankanderson · 2025-06-25T22:35:36Z

injection/sharedmain/main.go

+		logger.Fatalw("Failed to setup trace provider", zap.Error(err))
+	}
+
+	otel.SetTextMapPropagator(tracing.DefaultTextMapPropagator())


This feels like it should be part of tracing.NewTracerProvider, even though it's actually a global spooky-action-at-a-distance manipulation. WDYT?

tracing.NewTracerProvider

That doesn't actually set the global tracer in NewTracerProvider. We do that in sharedmain after setting the SetTextMapPropagator.

I prefer the observability package to not set any OTel globals.

evankanderson · 2025-06-25T22:37:09Z

injection/sharedmain/main.go

+	if _, err := client.Get(ctx, cmName, metav1.GetOptions{}); err == nil {
+		cmw.Watch(cmName, observers...)
+	} else if !apierrors.IsNotFound(err) {
+		logger.Fatalw("Error reading ConfigMap "+cmName, zap.Error(err))


I'm assuming we don't have a way to notice if the ConfigMap shows up later?

This file could use a cleanup - I've just changed the 'Watch' here but the diff shows a larger change.

Generally, I think the controllers should halt and wait if a config map is not present. I could imagine a split brain scenario occurring.

I'd rather not make those edits in this PR.

That's fine. I'm slighly worried about "halt and catch fire" as a behavior when a configmap we ship has been removed. The idea of shipping them has mostly been about documentation, not about requiring them to have specific contents (or even be present) for things to function.

injection/sharedmain/pprof.go

dprotaso · 2025-06-26T17:51:42Z

I'm marking this ready - we can merge this and it will 'break' metrics/tracing @ HEAD - but I think that's worth it since it'll be easier to review and make follow up changes.

Plus metrics/tracing would have already been 'broken' downstream if we merged pkg changes

dprotaso · 2025-06-26T17:57:21Z

observability/resource/default.go

+
+	// Ignore the error because it complains about semconv
+	// schema version differences
+	resource, _ := resource.Merge(


there shouldn't be an error here anymore since the telemetry sdk attributes and resource now use the same semconv version.

But before if the semconv packages were different it would return this error with a resource without a schema
https://github.com/open-telemetry/opentelemetry-go/blob/957f4417b7719eeff17f051228520176fd2d5b78/sdk/resource/resource.go#L218

There was a bug in v1.36.0 where they didn't match up which was odd.

I added a test and now we panic if there is an error.

We can always change this in the future if we find it annoying

dprotaso · 2025-06-26T17:58:01Z

/hold

Actually I want to make another related change

Cali0707

Just a few nits, but overall lgtm

observability/resource/default.go

system/env.go

Given Views in OTel are overrides for end-users this should really be a sharedmain option instead of a dedicated package. In OpenCensus views were required but in OTel they are not

dprotaso · 2025-06-26T18:35:24Z

Talking to the OTel folks - views are an 'end-user' override. Unlike OpenCensus they are not mandatory to get metrics export to work.

So it seems more fitting to remove the global package I created and just make it a sharedmain option that controller authors can change.

dprotaso · 2025-06-26T18:43:23Z

/hold cancel

Cali0707

/lgtm
/hold in case @dprotaso wants more feedback from others

knative-prow · 2025-06-26T20:06:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

evankanderson

I'm okay with this as-is, but I had a couple comments that didn't get sent this morning.

evankanderson · 2025-06-26T12:53:00Z

injection/sharedmain/main.go

+	if _, err := client.Get(ctx, cmName, metav1.GetOptions{}); err == nil {
+		cmw.Watch(cmName, observers...)
+	} else if !apierrors.IsNotFound(err) {
+		logger.Fatalw("Error reading ConfigMap "+cmName, zap.Error(err))


That's fine. I'm slighly worried about "halt and catch fire" as a behavior when a configmap we ship has been removed. The idea of shipping them has mostly been about documentation, not about requiring them to have specific contents (or even be present) for things to function.

injection/sharedmain/main.go

injection/sharedmain/pprof.go

…oes it

evankanderson

/lgtm

dprotaso · 2025-06-27T00:17:08Z

/hold cancel

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 25, 2025

knative-prow bot requested review from Cali0707 and skonto June 25, 2025 13:11

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2025

dprotaso force-pushed the observability-sharedmain branch from 8fcfcd7 to 174f42a Compare June 25, 2025 13:39

dprotaso mentioned this pull request Jun 25, 2025

New observability package #3188

Merged

dprotaso force-pushed the observability-sharedmain branch from 174f42a to 412b5ee Compare June 25, 2025 14:18

dprotaso mentioned this pull request Jun 25, 2025

[webhook] OTel changes #3189

Merged

dprotaso force-pushed the observability-sharedmain branch 2 times, most recently from 1135ba7 to b7cc628 Compare June 25, 2025 15:08

evankanderson reviewed Jun 25, 2025

View reviewed changes

dprotaso added 3 commits June 26, 2025 00:09

sharedmain changes

6050971

go mod

5a8efc8

vendor

db3e763

dprotaso force-pushed the observability-sharedmain branch from b7cc628 to db3e763 Compare June 26, 2025 04:09

address PR feedback

8942a75

dprotaso changed the title ~~[wip] OTel injection/sharedmain changes~~ OTel injection/sharedmain changes Jun 26, 2025

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 Jun 26, 2025

bump semconv to v1.34.0

5d844f5

dprotaso commented Jun 26, 2025

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025

Cali0707 reviewed Jun 26, 2025

View reviewed changes

observability/resource/default.go Outdated Show resolved Hide resolved

system/env.go Outdated Show resolved Hide resolved

system/env.go Outdated Show resolved Hide resolved

dprotaso added 3 commits June 26, 2025 14:28

drop globalviews in favour of sharedmain option

74d33d8

Given Views in OTel are overrides for end-users this should really be a sharedmain option instead of a dedicated package. In OpenCensus views were required but in OTel they are not

PR feedback

638adc7

panic if we drift semconv version from the SDK

23703f7

dprotaso added 2 commits June 26, 2025 14:41

fix testing import

12eb736

bump runtime instrumentation

c451e69

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025

dprotaso changed the title ~~OTel injection/sharedmain changes~~ [injection/sharedmain] OTel Support Jun 26, 2025

gofumpt file

710ae45

Cali0707 approved these changes Jun 26, 2025

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025

knative-prow bot assigned Cali0707 Jun 26, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2025

evankanderson reviewed Jun 26, 2025

View reviewed changes

don't set enabled=false on init since the profiling Handler already d…

db62f56

…oes it

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2025

dprotaso mentioned this pull request Jun 27, 2025

Consider changing sharedmain behaviour about panic'ing when a config map is missing #3195

Open

evankanderson reviewed Jun 27, 2025

View reviewed changes

knative-prow bot assigned evankanderson Jun 27, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2025

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2025

knative-prow bot merged commit 5abfb10 into knative:main Jun 27, 2025
36 of 38 checks passed

dprotaso deleted the observability-sharedmain branch June 27, 2025 00:19

[injection/sharedmain] OTel Support #3190

[injection/sharedmain] OTel Support #3190

Uh oh!

Conversation

dprotaso commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evankanderson Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dprotaso commented Jun 26, 2025

Uh oh!

dprotaso Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Jun 26, 2025

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dprotaso commented Jun 26, 2025

Uh oh!

dprotaso commented Jun 26, 2025

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Jun 26, 2025

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Jun 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dprotaso commented Jun 25, 2025 •

edited

Loading

codecov bot commented Jun 25, 2025 •

edited

Loading

dprotaso Jun 26, 2025 •

edited

Loading