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.

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

Use transformers efficiently for extracting info for metrics in Kafka Channel #5505

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Use transformers efficiently for extracting info for metrics in Kafka Channel #5505

merged 3 commits into from
Jun 14, 2021

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Jun 14, 2021 •
edited
Loading

In knative-extensions/eventing-kafka#688 the Kafka message needs to be copied to in order to extract the event type (used for labeling metrics) with a transformer. Transformers can be used though in DispatchWithRetries via the execute method, so with this PR we can touch the msg only when we write it and get the info we need back efficiently.
Implementation example of this can be found here downstream.

/cc @matzew

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 14, 2021
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 14, 2021
@skonto
Copy link
Contributor Author

skonto commented Jun 14, 2021

/assign @slinkydeveloper

@skonto skonto changed the title Use transformers for extracting event type for metrics Use transformers efficiently for extracting event type for metrics Jun 14, 2021
@skonto skonto changed the title Use transformers efficiently for extracting event type for metrics Use transformers efficiently for extracting event type for metrics in Kafka Channel Jun 14, 2021
slinkydeveloper
slinkydeveloper reviewed Jun 14, 2021
View reviewed changes
pkg/channel/fanout/fanout_message_handler.go Outdated
@@ -304,6 +304,7 @@ func (f *FanoutMessageHandler) makeFanoutRequest(ctx context.Context, message bi
sub.Reply,
sub.DeadLetter,
sub.RetryConfig,
nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this

@skonto skonto changed the title Use transformers efficiently for extracting event type for metrics in Kafka Channel Use transformers efficiently for extracting info for metrics in Kafka Channel Jun 14, 2021
@codecov
Copy link

codecov bot commented Jun 14, 2021 •
edited
Loading

Codecov Report

Merging #5505 (a94d63c) into main (03c2a69) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5505   +/-   ##
=======================================
  Coverage   82.74%   82.74%           
=======================================
  Files         198      198           
  Lines        6098     6098           
=======================================
  Hits         5046     5046           
  Misses        727      727           
  Partials      325      325
Impacted Files Coverage Δ
pkg/channel/message_dispatcher.go 78.51% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03c2a69...a94d63c. Read the comment docs.

@slinkydeveloper
Copy link
Contributor

Can you add the rest of the implementation in this pr?

@skonto
Copy link
Contributor Author

skonto commented Jun 14, 2021

Which part? I am planning to create a PR to update eventing-kafka after this.

@slinkydeveloper
Copy link
Contributor

@skonto you pass the transformers in the signature, but you aren't using them, right?

@skonto
Copy link
Contributor Author

skonto commented Jun 14, 2021

Oh yes missed that part will fix in a sec

@skonto
Copy link
Contributor Author

skonto commented Jun 14, 2021 •
edited
Loading

@slinkydeveloper Codegen issue is a known one? It is a git file permission issue hmm.

@slinkydeveloper
Copy link
Contributor

@slinkydeveloper Codegen issue is a known one? It is a git file permission issue hmm.

Yeah it doesn't seem related to your PR, I'm experiencing it too

@skonto
Copy link
Contributor Author

skonto commented Jun 14, 2021 •
edited
Loading 

$ git ls-files -s third_party/VENDOR-LICENSE/github.com/hashicorp/go-retryablehttp/README.md
100644 30357c75668cb7efddb57bf95d7da89bc16ae4ec 0	third_party/VENDOR-LICENSE/github.com/hashicorp/go-retryablehttp/README.md

Locally it seems ok. So it seems that codegen modifies git file permissions somehow.

Following failures are unrelated:

  • RabbitMq
  • Conformance
  • e2e tests:
--- FAIL: TestBinaryEventForChannel (74.98s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Setup/install_sink (28.43s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Setup/install_channel (4.18s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Setup/install_subscription (4.62s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Setup/subscription_is_ready (1.97s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Setup/channel_is_ready (2.22s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Setup/install_source (27.42s)
    --- PASS: TestBinaryEventForChannel/SingleEventWithEncoding/Requirement/sender_is_finished (3.00s)
    --- FAIL: TestBinaryEventForChannel/SingleEventWithEncoding/Assert/[Stable/MUST]sink_receives_events (0.00s)

@skonto
Copy link
Contributor Author

skonto commented Jun 14, 2021

/retest

slinkydeveloper
slinkydeveloper approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto, 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 Jun 14, 2021
@knative-prow-robot knative-prow-robot merged commit b0b32c4 into knative:main Jun 14, 2021
This was referenced Jun 14, 2021
Closed
Merged
@lionelvillard lionelvillard mentioned this pull request Jun 15, 2021
Merged
skonto added a commit to skonto/eventing that referenced this pull request Jun 16, 2021
@skonto
Use transformers efficiently for extracting info for metrics in Kafka…
91c5291 
… Channel (knative#5505)

* use transformers for extracting event type for metrics

* fix

* fix
openshift-merge-robot pushed a commit to openshift/knative-eventing that referenced this pull request Jun 17, 2021
@skonto
[Backport] [Release 0.23] Use transformers to extract metrics for Kaf…
f7036f2 
…ka (#1318)

* make eventing metrics generic (knative#5478)

* Expose dispatch result to be used by other implementations (knative#5481)

* expose dispatch result to be used by other implementations

* expose fields

* add getters etc

* less verbose

* Use transformers efficiently for extracting info for metrics in Kafka Channel  (knative#5505)

* use transformers for extracting event type for metrics

* fix

* fix

* pass transformers to executeRequest (knative#5512)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slinkydeveloper slinkydeveloper slinkydeveloper approved these changes

Assignees

@slinkydeveloper slinkydeveloper

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skonto @slinkydeveloper @knative-prow-robot