Adding data plane metrics to ApiServerSource#1786
Conversation
knative-prow-robot
added
size/XXL
|
Hi @sayanh. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
knative-prow-robot
added
the
area/test-and-release
|
/hold |
knative-prow-robot
added
the
do-not-merge/hold
|
/ok-to-test |
knative-prow-robot
added
ok-to-test
sayanh
force-pushed
the
add-http-response-codes-in-metrics
branch
from
df4492a to
0eca934
Compare
|
can you update the title of the PR to better match what is it about. Adding data plane metrics to ApiServerSource, maybe? |
sayanh
force-pushed
the
add-http-response-codes-in-metrics
branch
from
3b3344c to
f73ddce
Compare
|
/lgtm |
| // metrics.ExporterOptions. This is used to configure the metrics exporter | ||
| // options, the config is stored in a config map inside the controllers | ||
| // namespace and copied here. | ||
| MetricsConfigBase64 string `envconfig:"K_METRICS_CONFIG" required:"true"` |
There was a problem hiding this comment.
Probably a foolish question, but why are we base64 encoding these? Flags tend to make sense, as it can be very annoying to get the escaping just right, but these are environment variables, where we don't have that problem.
| panic(err) | ||
| } | ||
| } | ||
| logger, _ := logging.NewLoggerFromConfig(loggingConfig, component) |
There was a problem hiding this comment.
Is this ignoring a possible error?
There was a problem hiding this comment.
No, this is ignoring a zap.AtomicLevel
| panic(err) | ||
| } | ||
| } | ||
| logger, _ := logging.NewLoggerFromConfig(loggingConfig, component) |
There was a problem hiding this comment.
Desugar the logger. As-is, there is a mistake on line 140. By desugaring, we can use the type system to ensure we don't have any problems.
There was a problem hiding this comment.
Fixed logger to be desugared
| rctx, _, err := a.ce.Send(ctx, *event) | ||
| rtctx := cloudevents.HTTPTransportContextFrom(rctx) | ||
| a.reporter.ReportEventCount(reportArgs, rtctx.StatusCode) | ||
| return err |
There was a problem hiding this comment.
Is a non-nil err logged in the caller?
There was a problem hiding this comment.
Added logs for non-nil error in here
| } | ||
|
|
||
| func makeRefAndTestingClient() (*ref, *kncetesting.TestCloudEventsClient) { | ||
| func makeRefAndTestingClient(t *testing.T) (*ref, *kncetesting.TestCloudEventsClient) { |
There was a problem hiding this comment.
t is not needed. I removed it.
|
|
||
| source string | ||
| sinkReconciler *duck.SinkReconciler | ||
| context context.Context |
There was a problem hiding this comment.
I don't think I've ever seen context saved this way. Can we rename this loggingContext or something similar?
There was a problem hiding this comment.
Right, renamed to loggingContext
| quoted64 := strconv.Quote(string(base64)) | ||
|
|
||
| var bytes []byte | ||
| // Do not care about the unmarshal error. |
There was a problem hiding this comment.
I don't understand this comment, if the error is non-nil, we return it. So I think we care about the error?
There was a problem hiding this comment.
We are indeed caring about the error. I removed those comments.
| return nil, err | ||
| } | ||
|
|
||
| // Do not care about the unmarshal error. |
| if err != nil { | ||
| return fmt.Sprintf(`{"error":"%s}`, err.Error()) | ||
| } | ||
| // Turn the base64 encoded []byte back into a string. |
There was a problem hiding this comment.
Seems like this belongs a few lines above.
|
|
||
| jsonOpts, err := json.Marshal(opts) | ||
| if err != nil { | ||
| return fmt.Sprintf(`{"error":"%s}`, err.Error()) |
There was a problem hiding this comment.
Why do this, rather than having the function return (string, error)?
There was a problem hiding this comment.
Absolutely, hence fixed
|
The following is the coverage report on pkg/.
|
| type mockReporter struct{} | ||
|
|
||
| func (r *mockReporter) ReportEventCount(args *ReportArgs, responseCode int) error { | ||
| return nil |
There was a problem hiding this comment.
I would still prefer if we asserted the metrics were incremented in the test, but I won't hold up this PR for it.
| metricsConfig, err := utils.Base64ToMetricsOptions( | ||
| env.MetricsConfigBase64) | ||
| if err != nil { | ||
| logger.Error("failed to process metrics options ", zap.Error(err)) |
There was a problem hiding this comment.
| logger.Error("failed to process metrics options ", zap.Error(err)) | |
| logger.Error("failed to process metrics options", zap.Error(err)) |
Don't need the extra space when using the desugared logger. Not here or the other lines.
Once, again, I'll let it submit as-is, please send a follow up PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Harwayne, sayanh 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 |
knative-prow-robot
added
the
approved
…VE-2022-21698 (knative#1786) * bump prom client to 1.11.1 for CVE-2022-21698 * vendor * third party
7 participants
Helps #1693
Proposed Changes
Release Note