[webhook] OTel changes #3189

dprotaso · 2025-06-25T13:05:37Z

This stacks onto the shared main PR - #3190

You can review the webhook changes here

webhook changes for observability
go mod
vendor

dprotaso · 2025-06-25T13:06:39Z

/assign @Cali0707 @evankanderson @skonto

codecov · 2025-06-26T04:25:40Z

Codecov Report

Attention: Patch coverage is 88.69565% with 13 lines in your changes missing coverage. Please review.

Project coverage is 76.00%. Comparing base (7a5377f) to head (624cd94).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
webhook/conversion.go	68.57%	8 Missing and 3 partials ⚠️
webhook/metrics.go	91.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3189      +/-   ##
==========================================
- Coverage   76.05%   76.00%   -0.05%     
==========================================
  Files         205      205              
  Lines       11751    11710      -41     
==========================================
- Hits         8937     8900      -37     
+ Misses       2541     2540       -1     
+ Partials      273      270       -3

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

dprotaso · 2025-06-30T18:36:29Z

This is ready for a review cc @evankanderson @Cali0707

Based on the feedback from the OTel folks I removed the package variable instruments. This makes unit tests easier to write and FYI the instruments are de-duped by the otel-go sdk when multiple webhooks are created (excluding the pointers in our webhook struct)

skonto · 2025-07-02T08:50:17Z

webhook/admission.go

+		attrs = append(attrs, allowedAttr)
+		labeler.Add(allowedAttr)
+
+		wh.metrics.recordHandlerDuration(ctx,


We report time for bad requests too?

Bad requests may have a different time distribution than "good" requests, which can be a useful hint when debugging, so I'm +1 on separating these.

webhook/webhook.go

webhook/webhook_test.go

evankanderson

/approve

One concern about use of LabelerFromContext without looking at the second value in admission.go, but this seems like (not counting libraries), this somewhat simplifies our overall code.

evankanderson · 2025-07-02T19:40:22Z

webhook/admission.go

+		attrs = append(attrs, allowedAttr)
+		labeler.Add(allowedAttr)
+
+		wh.metrics.recordHandlerDuration(ctx,


Bad requests may have a different time distribution than "good" requests, which can be a useful hint when debugging, so I'm +1 on separating these.

evankanderson · 2025-07-02T19:41:58Z

webhook/admission.go

 		}
 		r.Body = io.NopCloser(&bodyBuffer)

+		labeler, _ := otelhttp.LabelerFromContext(r.Context())


If the second result is false, then:

In this case it is safe to use the Labeler but any attributes added to it will not be used.

Do we want / need to call otelhttp.ContextWithLabeler to set the labeler if this was false?

shouldn't need to since - the otelhttp middleware sets it.

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/45af9252727c122174f7adab90f6c24ab04f1a6b/instrumentation/net/http/otelhttp/handler.go#L176

If someone's using the handler without that middleware then the labeler is a noop

Yeah, I didn't know if that was a concern. You can add a comment here that this relies on that middleware setting the labeler context as another option, since this looks like ignoring an error value.

webhook/admission.go

webhook/admission_integration_test.go

webhook/conversion.go

evankanderson · 2025-07-02T19:52:49Z

webhook/conversion.go

+		labeler.Add(
+			ConversionResultStatus.With(strings.ToLower(response.Response.Result.Status)),
+		)


Why do this after the recordHandlerDuration call?

recordHandlerDuration will add the duration to our custom histogram metric that measures the call to the conversion controller.

The labeler on the other hand will add attributes that otelhttp middelware reports. The the order of these don't really mater in my mind

So the labeler labels won't have any relation to the metric attributes?

So the labeler labels won't have any relation to the metric attributes?

There is no relationship. What we should probably do is actually get the attribute keys from the label when recording the metric - that way we'll have the exact same attributes (though there's a slice alloc)

ok - i moved this invocation upward and record will now fetch the labeler attributes

evankanderson · 2025-07-02T19:54:23Z

webhook/metrics.go

+	AdmissionOperation   = attributekey.String("kn.webhook.admission.operation")
+	AdmissionGroup       = attributekey.String("kn.webhook.admission.group")
+	AdmissionVersion     = attributekey.String("kn.webhook.admission.version")
+	AdmissionKind        = attributekey.String("kn.webhook.admission.kind")
+	AdmissionSubresource = attributekey.String("kn.webhook.admission.subresource")
+	AdmissionAllowed     = attributekey.Bool("kn.webhook.admission.result.allowed")


Do we need admission in these keys? If not, they could cover all kinds of webhook metrics (conversion, defaulting, etc), with fewer definitions. (e.g. the admission and conversion status could use the same attribute)

It's possible but a lot of the fields don't apply to conversion webhooks (maybe just two).

What are your concerns?

Having to type a lot of long strings into Prometheus queries. (Yes, it's more human laziness than technical cost, which should be fairly minimal with a reasonable interning implementation.)

ok - i did a pass take a look - i think it worked out

Cali0707

/lgtm
/hold in case @dprotaso wants any more reviews

…o unit test Digging into OTel implementation it looks like instruments are cached. Thus when different webhooks request the same instrument a cache lookup for the instrument will occur.

…rom the labeler

dprotaso · 2025-07-04T14:45:41Z

rebased

Cali0707

/lgtm

knative-prow · 2025-07-04T14:49:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, 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~~ [Cali0707,dprotaso]

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

dprotaso · 2025-07-04T14:51:18Z

/hold cancel

@evankanderson if you have any follow up comments let me know and i can do them in a follow up PR next week.

knative-prow bot requested review from creydr and skonto June 25, 2025 13:05

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 changed the title ~~Webhook OTel changes~~ [wip] Webhook OTel changes Jun 25, 2025

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 25, 2025

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

dprotaso mentioned this pull request Jun 25, 2025

[injection/sharedmain] OTel Support #3190

Merged

dprotaso force-pushed the observability-webhook branch from 40c7046 to a3884bc Compare June 25, 2025 13:38

dprotaso mentioned this pull request Jun 25, 2025

New observability package #3188

Merged

dprotaso force-pushed the observability-webhook branch 5 times, most recently from f32c228 to 62402c6 Compare June 26, 2025 04:19

dprotaso force-pushed the observability-webhook branch 2 times, most recently from 0977ab8 to bc10cae Compare June 30, 2025 18:32

dprotaso changed the title ~~[wip] Webhook OTel changes~~ Webhook OTel changes Jun 30, 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 30, 2025

skonto reviewed Jul 2, 2025

View reviewed changes

webhook/webhook.go Outdated Show resolved Hide resolved

skonto reviewed Jul 2, 2025

View reviewed changes

webhook/webhook_test.go Outdated Show resolved Hide resolved

dprotaso changed the title ~~Webhook OTel changes~~ [webhook] OTel changes Jul 2, 2025

dprotaso mentioned this pull request Jul 2, 2025

Drop OpenCensus use in various test packages #3202

Merged

evankanderson reviewed Jul 2, 2025

View reviewed changes

Cali0707 approved these changes Jul 4, 2025

View reviewed changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 4, 2025

dprotaso added 15 commits July 4, 2025 10:42

webhook changes for observability

45b35ce

go mod

526eb3c

vendor

f75c25c

Allow the webhook to accept a MetricsProvider. This makes it easier t…

3fe4c81

…o unit test Digging into OTel implementation it looks like instruments are cached. Thus when different webhooks request the same instrument a cache lookup for the instrument will occur.

passthrough additional OTel interfaces

ce06fab

linter fixes

9c17b24

drop global meter in tests

fed5395

PR feedback

6f37614

PR feedback

48df033

when recording the admission/conversion duration use the attributes f…

d9f7cd3

…rom the labeler

add a comment that otelhttp middleware creates the labeler

525b7ed

simply code for readability

fd68d74

change metric names to allow reuse across different webhook types

4dcf1bf

fix linter

126f9fb

tweak attribute names

624cd94

dprotaso force-pushed the observability-webhook branch from 7687b56 to 624cd94 Compare July 4, 2025 14:44

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

Cali0707 approved these changes Jul 4, 2025

View reviewed changes

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

knative-prow bot merged commit f478764 into knative:main Jul 4, 2025
35 of 37 checks passed

dprotaso deleted the observability-webhook branch July 4, 2025 14:57

[webhook] OTel changes #3189

[webhook] OTel changes #3189

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!

dprotaso commented Jun 25, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dprotaso commented Jun 30, 2025

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Jul 4, 2025

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Jul 4, 2025

Uh oh!

dprotaso commented Jul 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dprotaso commented Jun 25, 2025 •

edited

Loading

codecov bot commented Jun 26, 2025 •

edited

Loading