Skip to content

Addressing tracing issues in FanoutMessageHandler & MessageReceiver #2862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 2, 2020
Merged

Addressing tracing issues in FanoutMessageHandler & MessageReceiver #2862

merged 5 commits into from
Apr 2, 2020

Conversation

slinkydeveloper
Copy link
Contributor

@slinkydeveloper slinkydeveloper commented Mar 30, 2020 •
edited
Loading

Also enabled logging on FanoutMessageHandler, in order to detect future flakyness on that test

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 30, 2020
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 30, 2020
@slinkydeveloper
Copy link
Contributor Author

/retest pull-knative-eventing-unit-tests

@slinkydeveloper
Copy link
Contributor Author

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 30, 2020
@slinkydeveloper
Copy link
Contributor Author

/test pull-knative-eventing-unit-tests

3 similar comments
@slinkydeveloper
Copy link
Contributor Author

/test pull-knative-eventing-unit-tests

@slinkydeveloper
Copy link
Contributor Author

/test pull-knative-eventing-unit-tests

@slinkydeveloper
Copy link
Contributor Author

/test pull-knative-eventing-unit-tests

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 30, 2020
@slinkydeveloper slinkydeveloper changed the title Enabled the logger in FanoutMessageHandler_ServeHTTP test Addressing tracing issues in FanoutMessageHandler & MessageReceiver Mar 30, 2020
slinkydeveloper
slinkydeveloper commented Mar 30, 2020
View reviewed changes
pkg/channel/fanout/fanout_message_handler.go Outdated
@@ -164,5 +164,8 @@ func (f *MessageHandler) makeFanoutRequest(ctx context.Context, message binding.
if sub.DeadLetterSinkURI != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this part makes sense? I'm not sure I understood how to use the duck type Destination and I don't get if the Subscribable should contain the DeadLetterSinkURI in sub.Delivery.DeadLetterSink, sub.DeadLetterSinkURI or both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2876 should address it

slinkydeveloper
slinkydeveloper commented Mar 30, 2020
View reviewed changes
pkg/tracing/traceparent.go Outdated

// ParseSpanFromTraceparentAttribute works the same as AddSpanFromTraceparentAttribute, but takes
// the extension value as function parameter
func ParseSpanFromTraceparentAttribute(ctx context.Context, name string, tp string) (context.Context, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this method to extract the traceparent directly from the kafka message in KafkaChannel

@slinkydeveloper
Copy link
Contributor Author

/assign @n3wscott

@slinkydeveloper slinkydeveloper force-pushed the messagehandler_war_part_2 branch from 723b06d to 703a4a4 Compare March 31, 2020 06:21
@slinkydeveloper
Copy link
Contributor Author

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2020
@slinkydeveloper slinkydeveloper mentioned this pull request Mar 31, 2020
Merged
@slinkydeveloper
Copy link
Contributor Author

/hold

Waiting for #2876

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2020
@nlopezgi nlopezgi mentioned this pull request Apr 2, 2020
Merged
@slinkydeveloper
Enabled the damn logger to check where the flakyness lives
53a7330 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Trying to address the tracing thing
2a07544 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
I don't know why, but looks like we need this to make the e2e tests w…
ee609d2 
…orking

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Fixed response recorder usage
016a70c 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
removed code doubled by rebase
3d32eb7 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper force-pushed the messagehandler_war_part_2 branch from 428557b to 3d32eb7 Compare April 2, 2020 14:52
@slinkydeveloper
Copy link
Contributor Author

/assign @ian-mi

@knative-metrics-robot
Copy link

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/channel/message_dispatcher.go 85.9% 86.3% 0.4

@slinkydeveloper
Copy link
Contributor Author

/hold

Let me give a shot to the transformer that populates the span

@n3wscott
Copy link
Contributor

n3wscott commented Apr 2, 2020

/lgtm
/approve

@slinkydeveloper
Copy link
Contributor Author

/unhold
i'll do in a followup

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 2, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, slinkydeveloper

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2020
@knative-prow-robot knative-prow-robot merged commit fbd7f66 into knative:master Apr 2, 2020
@slinkydeveloper slinkydeveloper deleted the messagehandler_war_part_2 branch April 2, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Harwayne Harwayne Awaiting requested review from Harwayne

@nachocano nachocano Awaiting requested review from nachocano

Assignees

@ian-mi ian-mi

@n3wscott n3wscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@slinkydeveloper @knative-metrics-robot @n3wscott @knative-prow-robot @googlebot @ian-mi