Skip to content
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.

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

Tracing tests for Kafka Broker and Channel #2369

Merged

Conversation

mgencur
Copy link
Contributor

@mgencur mgencur commented Jul 1, 2022

Fixes #585

This PR still has vendor/knative.dev/pkg/test/tracing/traces.go in vendor directory. It's a copy of this file from knative/eventing with slight changes. I plan to send a PR to knative.dev/pkg and then vendor this file to this repository.
EDIT: Sent knative/eventing#6446 which was merged and this PR has been rebased.

Proposed Changes

  • Enable port-forwarding to Zipkin pod
  • Send an event through Kafka Broker
  • Pull the trace from Zipkin
  • Verify the Trace

Release Note


Docs

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 1, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 1, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@knative-prow knative-prow bot added area/test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2022
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #2369 (0b62f28) into main (26b7142) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2369      +/-   ##
============================================
- Coverage     64.62%   64.54%   -0.09%     
+ Complexity      709      708       -1     
============================================
  Files           146      146              
  Lines          9806     9806              
  Branches        224      224              
============================================
- Hits           6337     6329       -8     
- Misses         3037     3046       +9     
+ Partials        432      431       -1     
Flag Coverage Δ
java-unittests 81.10% <ø> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ive/eventing/kafka/broker/core/AsyncCloseable.java 66.66% <0.00%> (-13.34%) ⬇️
...dispatcher/impl/consumer/BaseConsumerVerticle.java 87.09% <0.00%> (-6.46%) ⬇️
...lane/pkg/reconciler/consumergroup/consumergroup.go 69.49% <0.00%> (-1.16%) ⬇️
...broker/dispatcher/impl/consumer/OffsetManager.java 94.62% <0.00%> (-1.08%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@mgencur mgencur force-pushed the broker_tracing_tests_main branch from 5d51cd4 to d595fbb Compare July 1, 2022 05:24
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2022
@mgencur mgencur changed the title [WIP] Tracing tests for Kafka Broker [WIP] Tracing tests for Kafka Broker and Channel Jul 1, 2022
@mgencur
Copy link
Contributor Author

mgencur commented Jul 11, 2022

This PR needs to wait for an updated dependency on knative/eventing (#2357)
Edit: Done

@mgencur mgencur changed the title [WIP] Tracing tests for Kafka Broker and Channel Tracing tests for Kafka Broker and Channel Jul 12, 2022
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 12, 2022
@mgencur
Copy link
Contributor Author

mgencur commented Jul 12, 2022

/retest-required

1 similar comment
@mgencur
Copy link
Contributor Author

mgencur commented Jul 12, 2022

/retest-required

@mgencur
Copy link
Contributor Author

mgencur commented Jul 15, 2022

@pierDipi Hi! This one is ready for review. Whenever you have time. Thanks :)

@@ -0,0 +1,158 @@
// Protocol Buffers - Google's data interchange format
Copy link
Member

Choose a reason for hiding this comment

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

I didn't fully get why we have all of these proto files, would you mind expanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pierDipi these files got in by running ./hack/update-codegen.sh. They're probably not related to this PR. Let me rebase and see if this is still required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. The codegen check now passes without those files. So, this PR doesn't include them anymore.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2022
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2022
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 1, 2022
@mgencur
Copy link
Contributor Author

mgencur commented Aug 1, 2022

/retest-required

Copy link
Member

@aliok aliok left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022
@knative-prow
Copy link

knative-prow bot commented Aug 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, mgencur

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 knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2022
@knative-prow knative-prow bot merged commit 62d9885 into knative-extensions:main Aug 1, 2022
@mgencur
Copy link
Contributor Author

mgencur commented Aug 1, 2022

@aliok @pierDipi thanks!

@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2022

Thank you @mgencur!

mgencur added a commit to mgencur/eventing-kafka-broker that referenced this pull request Aug 2, 2022
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this pull request Aug 2, 2022
knative-prow bot pushed a commit that referenced this pull request Aug 2, 2022
* Tracing tests for Kafka Broker (#2369)

* Manually update deps

./hack/update-deps.sh --upgrade --release 1.5 --domain
knative.dev/eventing
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this pull request Aug 2, 2022
openshift-ci bot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Aug 3, 2022
* OpenShift Script changes

* Tracing tests for Kafka Broker (knative-extensions#2369)

* Manually update deps

./hack/update-deps.sh --upgrade --release 1.5 --domain
knative.dev/eventing

* Reuse tracing and KnativeKafka installation from serverless-operator
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this pull request Aug 17, 2022
knative-prow bot pushed a commit that referenced this pull request Aug 18, 2022
* Tracing tests for Kafka Broker (#2369)

* Manually update deps

./hack/update-deps.sh --upgrade --release 1.6 --domain knative.dev/eventing

* Fix formatting for sacura_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants