Minor refactor for broker ingress, adding unittests. #2270

n3wscott · 2019-12-05T20:00:34Z

Found an issue where the broker filter and ingress report metrics with the same name, preventing go coverage from running.
Introduce an event defaulter to wrap the management of TTL for broker ingress. This centralizes the logic of dealing with TTL, deleting some code.

Release Note

NONE

knative-prow-robot · 2019-12-05T20:00:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [n3wscott]

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

nachocano · 2019-12-05T20:10:20Z

pkg/broker/filter/stats_reporter.go

@@ -38,23 +38,23 @@ var (
 	// eventCountM is a counter which records the number of events received
 	// by a Trigger.
 	eventCountM = stats.Int64(
-		"event_count",
+		"broker_filter_event_count",


this will break stackdriver integration... these metrics names are whitelisted in SD so that nobody is charged

It is wrong as is as they conflict. Knative does not have knowledge of stack driver whitelists.

I guess now it has :)
Metrics are API surface. We cannot just change them because code coverage cannot run...

nachocano · 2019-12-05T20:12:23Z

pkg/broker/ingress/stats_reporter.go

@@ -33,15 +33,15 @@ var (
 	// eventCountM is a counter which records the number of events received
 	// by the Broker.
 	eventCountM = stats.Int64(
-		"event_count",
+		"broker_ingress_event_count",


same thing here... why do we need this change? is for a UT coverage thing?
we are matching the naming convention of metrics in serving..

Code coverage can not run with the two init methods registering the two matching names. Plus this prevents us from running in the same container...

code coverage is not a blocker for merging things. If there is a workaround without changing the names, then go for it... Otherwise we should not change the names.

When the time comes to run in the same container, then I think we can revisit.

n3wscott · 2019-12-05T21:00:52Z

@nachocano I think it is not valid to hold a PR because it would break private integration that is not part of Knative.

nachocano · 2019-12-05T21:09:17Z

@n3wscott it's not private integration, everyone can use SD. See pkg, serving has all the metrics there, and we too (although it should be refactored).
Furthermore, we are using the same metric naming conventions as serving. Serving doesn't add prefixes, and neither we should.
Metrics are API surface, people have dashboards and are relying on metrics. We cannot just simply change them because "code coverage doesn't run".

n3wscott · 2019-12-05T21:12:27Z

Then we should use the registration that serving uses to prevent errors from the the view.Register call. As it is, it does not work.

nachocano · 2019-12-05T21:23:09Z

Then we should use the registration that serving uses to prevent errors from the the view.Register call. As it is, it does not work.

OK, that makes sense. Can you take care of that change in this PR?

n3wscott · 2019-12-05T21:26:06Z

Let me take a crack.

I don't want this merged before 0.11 is cut regardless.

nachocano · 2019-12-05T21:41:54Z

Let me take a crack.

Thanks!

I don't want this merged before 0.11 is cut regardless.

Yes, makes sense!

n3wscott · 2019-12-06T17:04:32Z

There is way more work that I want to start for refactoring the measurements code.

I am removing the panic and moving on.

nachocano · 2019-12-06T17:12:13Z

Cool! Yeah, I created some issues in pkg sometime back. That needs a refactor as well

…

On Fri, Dec 6, 2019 at 9:04 AM Scott Nichols ***@***.***> wrote: There is way more work that I want to start for refactoring the measurements code. I am removing the panic and moving on. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2270?email_source=notifications&email_token=ABD65DBHJR4P6Y3PIRVGX6TQXKA2DA5CNFSM4JV7STFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGEXFJQ#issuecomment-562655910>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DCUL4WUIRQZQASQXTDQXKA2DANCNFSM4JV7STFA> .

n3wscott · 2019-12-18T22:57:46Z

/retest
I reverted the issue that Nacho had with the previous PR.
/hold cancel

nachocano · 2019-12-18T23:16:50Z

pkg/broker/ingress/ingress_handler_test.go

 package ingress

 import (
 	"context"
+	"knative.dev/eventing/pkg/broker"


pkg/broker/ttl.go

nachocano · 2019-12-18T23:24:46Z

pkg/broker/ttl.go

+//   If TTL is not found, it will set it to the default passed in.
+//   If TTL is <= 0, it will remain 0.
+//   If TTL is > 1, it will be reduced by one.
+func TTLDefaulter(logger *zap.Logger, defaultTTL int32) client.EventDefaulter {


should we check defaultTTL is > 0?

nachocano · 2019-12-18T23:25:16Z

pkg/broker/ttl_test.go

+package broker
+
+import (
+	"context"


format imports?

nachocano · 2019-12-18T23:36:02Z

pkg/broker/ingress/ingress_handler.go

 	// Setting the extension as a string as the CloudEvents sdk does not support non-string extensions.
-	event.SetExtension(broker.EventArrivalTime, time.Now().Format(time.RFC3339))
+	event.SetExtension(broker.EventArrivalTime, cloudevents.Timestamp{Time: time.Now()})


can you make sure we are not breaking the way we are reading the extension on the filter pod?

eventing/pkg/broker/filter/filter_handler.go

Line 248 in b53d3fd

var arrivalTimeStr string

yeah it works.

nachocano · 2019-12-18T23:37:04Z

Very cosmetic comments, and one that I think is important, the arrival time extension.
Can you update the PR description and release notes as we are not adding that prefix anymore?

nachocano · 2019-12-18T23:48:08Z

pkg/broker/ttl.go

 		} else if ttl, err = cetypes.ToInteger(ttlraw); err != nil {
 			logger.Warn("Failed to convert existing TTL into integer, defaulting.",
 				zap.String("event.id", event.ID()),
 				zap.Any(TTLAttribute, ttlraw),
 				zap.Error(err),
 			)
-			ttl = defaultTTL + 1
+			ttl = defaultTTL
+		} else {


nachocano · 2019-12-18T23:49:25Z

/lgtm

knative-metrics-robot · 2019-12-18T23:53:38Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/broker/ingress/ingress_handler.go	60.0%	82.9%	22.9
pkg/broker/ttl.go	Do not exist	85.0%

Scott Nichols added 3 commits December 5, 2019 11:14

minor refactor for ttl management for broker ingress. More unit tests.

ddac7f8

broker metrics were conflicting.

faa7a3e

adding ttl defaulter tests.

4a0f56b

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 5, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 5, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2019

mattmoor reviewed Dec 5, 2019

View reviewed changes

knative-prow-robot requested review from lionelvillard and matzew December 5, 2019 20:00

Merge branch 'master' into broker-unittests

2f55176

mattmoor reviewed Dec 5, 2019

View reviewed changes

adding cr.

c146ebb

n3wscott force-pushed the broker-unittests branch from 96aa0c9 to c146ebb Compare December 5, 2019 20:04

nachocano reviewed Dec 5, 2019

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2019

nachocano reviewed Dec 5, 2019

View reviewed changes

fix race.

681b3f4

Do not panic if a view is already registered.

f696b2b

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2019

nachocano reviewed Dec 18, 2019

View reviewed changes

pkg/broker/ttl.go Outdated Show resolved Hide resolved

nachocano reviewed Dec 18, 2019

View reviewed changes

pkg/broker/ttl_test.go

package broker

import (

"context"

Copy link

Contributor

nachocano Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format imports?

nachocano reviewed Dec 18, 2019

View reviewed changes

Scott Nichols added 2 commits December 18, 2019 15:40

Merge branch 'master' into broker-unittests

ea9e8d0

fix imports.

8ce5089

nachocano reviewed Dec 18, 2019

View reviewed changes

knative-prow-robot assigned nachocano Dec 18, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2019

knative-prow-robot merged commit 5631d77 into knative:master Dec 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor refactor for broker ingress, adding unittests. #2270

Minor refactor for broker ingress, adding unittests. #2270

n3wscott commented Dec 5, 2019 •

edited

Loading

knative-prow-robot commented Dec 5, 2019

nachocano Dec 5, 2019

nachocano Dec 5, 2019

n3wscott Dec 5, 2019

nachocano Dec 5, 2019

nachocano Dec 5, 2019

n3wscott Dec 5, 2019

nachocano Dec 5, 2019

n3wscott commented Dec 5, 2019

nachocano commented Dec 5, 2019

n3wscott commented Dec 5, 2019

nachocano commented Dec 5, 2019

n3wscott commented Dec 5, 2019

nachocano commented Dec 5, 2019

n3wscott commented Dec 6, 2019

nachocano commented Dec 6, 2019 via email

n3wscott commented Dec 18, 2019

nachocano Dec 18, 2019

nachocano Dec 18, 2019

nachocano Dec 18, 2019

nachocano Dec 18, 2019

n3wscott Dec 18, 2019

nachocano commented Dec 18, 2019

nachocano Dec 18, 2019

nachocano commented Dec 18, 2019

knative-metrics-robot commented Dec 18, 2019

Minor refactor for broker ingress, adding unittests. #2270

Minor refactor for broker ingress, adding unittests. #2270

Conversation

n3wscott commented Dec 5, 2019 • edited Loading

knative-prow-robot commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3wscott commented Dec 5, 2019

nachocano commented Dec 5, 2019

n3wscott commented Dec 5, 2019

nachocano commented Dec 5, 2019

n3wscott commented Dec 5, 2019

nachocano commented Dec 5, 2019

n3wscott commented Dec 6, 2019

nachocano commented Dec 6, 2019 via email

n3wscott commented Dec 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nachocano commented Dec 18, 2019

Choose a reason for hiding this comment

nachocano commented Dec 18, 2019

knative-metrics-robot commented Dec 18, 2019

n3wscott commented Dec 5, 2019 •

edited

Loading