New observability package by dprotaso · Pull Request #3188 · knative/pkg

dprotaso · 2025-06-25T12:57:49Z

include observability package
go mod changes
commit vendor

dprotaso · 2025-06-25T12:58:04Z

/hold for review

dprotaso · 2025-06-25T12:58:29Z

/assign @Cali0707 @evankanderson @skonto

dprotaso · 2025-06-25T13:40:36Z

Follow up PRs are split up for review

webhook - #3189
sharedmain - #3190

dprotaso · 2025-06-25T15:03:08Z

new OTel libs were cut today - I just bumped them

codecov · 2025-06-25T15:05:45Z

Codecov Report

Attention: Patch coverage is 84.98099% with 79 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@fe85a65). Learn more about missing BASE report.
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
observability/metrics/provider.go	67.90%	20 Missing and 6 partials ⚠️
observability/tracing/provider.go	76.66%	16 Missing and 5 partials ⚠️
observability/metrics/metricstest/assert.go	82.35%	12 Missing ⚠️
observability/config.go	67.85%	6 Missing and 3 partials ⚠️
observability/metrics/prometheus/server.go	92.85%	4 Missing ⚠️
observability/attributekey/key.go	88.88%	3 Missing ⚠️
observability/metrics/prometheus_enabled.go	88.00%	2 Missing and 1 partial ⚠️
observability/metrics/config.go	97.14%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3188   +/-   ##
=======================================
  Coverage        ?   75.96%           
=======================================
  Files           ?      205           
  Lines           ?    11690           
  Branches        ?        0           
=======================================
  Hits            ?     8880           
  Misses          ?     2539           
  Partials        ?      271

☔ 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.

Cali0707 · 2025-06-25T19:12:23Z

+	return Config{
+		Protocol: ProtocolNone,
+	}


Should we default the export interval?

I think 0 for "do the right thing" makes sense.

I am wondering if we should allow "" to mean either "prometheus" or "none" (i.e. make empty string an acceptable value)...

either "prometheus" or "none"

I think that would cause confusion since it's not explicit

evankanderson

I'm assuming there's a future PR that adds interfaces for recording metrics, traces, etc, along with examples of usage.

evankanderson · 2025-06-25T21:23:17Z

Why not put this in the observability package directly (what users are going to use that package, but not configmap.Parse())?

I wanted to keep k8s api deps out of this since it includes a lot of types.

We'll be able to reduce queue proxy binary size: knative/serving#9957 (comment)

evankanderson · 2025-06-25T21:56:35Z

+	return &ProfilingServer{
+		ProfilingHandler: h,
+		Server: &http.Server{
+			Addr:    ":" + port,


For metrics / prometheus, you allow setting the listen address, but you don't here. Intentional?

Not intentional - just moving the existing profile server from metrics package into a separate one here.

evankanderson · 2025-06-25T22:24:25Z

+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.Background()
+			_, err := NewMeterProvider(ctx, tc.c)


If you set Endpoint for Prometheus (at least), you'll get a bit of extra code coverage. (You may also want to .validate() your configs.)

let me add a test for prometheus endpoint. (done)

You may also want to .validate() your configs.)

Can you clarify? Are you asking I call Validate on the cfg that's passed to NewMeterProvider?

dprotaso · 2025-06-26T02:28:31Z

flake - metrics: TestMetricsExport/OpenCensus

/retest

dprotaso · 2025-06-26T02:31:58Z

I'm assuming there's a future PR that adds interfaces for recording metrics, traces, etc, along with examples of usage.

Actually I don't think we need this - we can just use OTel libraries - see this diff on how I added a custom metric to the webhook. 8b5f38b

evankanderson

/approve

evankanderson · 2025-06-26T03:32:18Z

+
+var globalViews map[string][]metric.View = make(map[string][]metric.View)
+
+// Register should be called during package init


Is go package init order guaranteed to be single-threaded?

If not, you may need to lock this map or use an atomic map.

Yeah the spec has

https://go.dev/ref/spec#Package_initialization

followed by calling all init functions in the order they appear in the source, possibly in multiple files, as presented to the compiler.

Then in https://go.dev/ref/spec#Program_initialization

Package initialization—variable initialization and the invocation of init functions—happens in a single goroutine, sequentially, one package at a time.

knative-prow · 2025-06-26T03:32:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

evankanderson · 2025-06-26T04:03:28Z

/lgtm

dprotaso · 2025-06-26T04:04:03Z

/hold cancel

knative-prow Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 25, 2025

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 25, 2025

dprotaso requested a review from skonto June 25, 2025 12:58

knative-prow Bot requested review from aslom and creydr June 25, 2025 12:58

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

knative-prow Bot assigned Cali0707, evankanderson and skonto Jun 25, 2025

dprotaso force-pushed the observability branch from cea48d0 to 895d393 Compare June 25, 2025 13:01

dprotaso mentioned this pull request Jun 25, 2025

[webhook] OTel changes #3189

Merged

include observability package

d00cd9b

dprotaso force-pushed the observability branch from 895d393 to b28287e Compare June 25, 2025 13:37

dprotaso mentioned this pull request Jun 25, 2025

[injection/sharedmain] OTel Support #3190

Merged

dprotaso added 2 commits June 25, 2025 10:59

go mod changes

da8f1c6

commit vendor

e7141e4

dprotaso force-pushed the observability branch from b28287e to e7141e4 Compare June 25, 2025 15:00

Cali0707 reviewed Jun 25, 2025

View reviewed changes

evankanderson reviewed Jun 25, 2025

View reviewed changes

PR feedback

8a5ace8

evankanderson reviewed Jun 26, 2025

View reviewed changes

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 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 26, 2025

knative-prow Bot merged commit 5e2512c into knative:main Jun 26, 2025
52 of 54 checks passed

dprotaso deleted the observability branch June 26, 2025 04:05


		var globalViews map[string][]metric.View = make(map[string][]metric.View)

		// Register should be called during package init

Conversation

dprotaso commented Jun 25, 2025

Uh oh!

dprotaso commented Jun 25, 2025

Uh oh!

dprotaso commented Jun 25, 2025

Uh oh!

dprotaso commented Jun 25, 2025

Uh oh!

dprotaso commented Jun 25, 2025

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

dprotaso commented Jun 26, 2025

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow Bot commented Jun 26, 2025

Uh oh!

evankanderson commented Jun 26, 2025

Uh oh!

dprotaso commented Jun 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jun 25, 2025 •

edited

Loading

dprotaso Jun 26, 2025 •

edited

Loading