[controller] OTel instrumentation#3201
Conversation
knative-prow
Bot
added
size/XXL
|
/assign @skonto @evankanderson @Cali0707 |
dprotaso
force-pushed
the
observability-controller
branch
from
de0a3d5 to
2cf4973
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3201 +/- ##
==========================================
- Coverage 76.05% 75.30% -0.75%
==========================================
Files 205 206 +1
Lines 11751 11867 +116
==========================================
Hits 8937 8937
- Misses 2541 2650 +109
- Partials 273 280 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
/approve
One concern in the client-go metrics, but this overall makes sense, and I like re-using the existing k8s metrics infrastructure, even if the workqueue implementation feels verbose (I like the tools version better).
Cali0707
left a comment
Cali0707
left a comment
There was a problem hiding this comment.
/approve
Overall, this looks great - just one small nit about how we are getting the response status code for the attributes
| if code == "200" { | ||
| attrs = attribute.NewSet( | ||
| semconv.ServerAddressKey.String(host), | ||
| semconv.HTTPRequestMethodKey.String(strings.ToUpper(method)), | ||
| semconv.HTTPResponseStatusCodeKey.Int(200), | ||
| ) | ||
| } else if c, err := strconv.Atoi(code); err != nil { |
There was a problem hiding this comment.
@dprotaso is there any value to having a specific code path just for the code == "200" case?
As far as I can tell, the only difference between this block and the next is that we are manually setting the int value of the response status code in the attribute set, instead of using the parsed value. IMO, this would be a bit simpler to read if we just drop the special case for 200.
There was a problem hiding this comment.
is there any value to having a specific code path just for the code == "200" case?
I was trying to avoid the string parsing on the 'happy' path - if you feel it's too much we can revert an revisit if there are problems in the future
There was a problem hiding this comment.
That makes sense, let's keep it as is then. Maybe add a comment for why?
- use strconv.Atoi - don't set status code attribute if we can't parse it
dprotaso
force-pushed
the
observability-controller
branch
from
32c1ebe to
6b157c4
Compare
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Soon this will be gone - |
5 participants
This changes the controller package to drop the use of the OpenCensus stats reporter.
As highlighted in the design document we had an overlap in our controller custom metrics and the ones provided by client-go workqueue package. For now we are removing the custom metrics and just going to rely on the workqueue metrics.
Breakdown of changes