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

[receiver/kafkareceiver] Add settings to Kafka Receiver for group management facilities #33082

Merged
merged 14 commits into from
Jul 23, 2024

Conversation

strawgate
Copy link
Contributor

Previous PR was closed due to inactivity waiting for review by maintainer: #32393

Description:

To address #28630 i've wired in the session timeout and heartbeat interval settings. I would have exposed max.poll.interval as well but it appears Sarama doesn't make that setting available. It has a similar setting called MaxProcessingTime that we could implement if desired but behavior is different. Something to note is that Kafka recommends a 45s session timeout for consumers but Samara actually defaults to 10s so I've also defaulted to 10s to keep behavior consistent.

Link to tracking Issue: #28630

Testing:

I started the OTel collector with the following configuration and verified via debugger that the settings were applied to the Sarama client.

receivers:
  kafka:
    topic: test
    session_timeout: 15s
    heartbeat_interval: 5s

exporters:
  debug:

service:
  pipelines:
    logs:  # a pipeline of “traces” type
      receivers: [kafka]
      exporters: [debug]

I then added a test to config_test that tests the default values and tests providing the values via config.

Documentation:

I modified the readme to reference the new settings.

@MovieStoreGuy
Copy link
Contributor

Hey @strawgate ,

Can I ask you to fix up the unit tests?

@strawgate
Copy link
Contributor Author

Yes will tackle soon, sorry i didn't catch that earlier

@MovieStoreGuy
Copy link
Contributor

Yes will tackle soon, sorry i didn't catch that earlier

Can I ask you to ping me once you have a chance to fix?

@strawgate
Copy link
Contributor Author

Yes will tackle soon, sorry i didn't catch that earlier

Can I ask you to ping me once you have a chance to fix?

@MovieStoreGuy good to go!

@ben-childs-docusign
Copy link
Contributor

ben-childs-docusign commented May 23, 2024

Thank you!

We tested this PR in our development environment and saw a significant improvement in our telemetry. Much less partition movement between consumers going from 10s timeout to 60s. Looking forward to seeing this merged.

@strawgate
Copy link
Contributor Author

Commenting to keep pr from going stale

@ben-childs-docusign
Copy link
Contributor

@MovieStoreGuy , @pavolloffay - any chance of getting this one approved and merged soon? We have this patch locally but it would be great to sync back up with the official builds.

@ben-childs-docusign
Copy link
Contributor

@MovieStoreGuy is there anything remaining to get this PR approved? This is making a huge improvement in the resource usage of our opentelemetry collectors so it will be great to have this merged into the official builds.

@strawgate
Copy link
Contributor Author

bump

@ben-childs-docusign
Copy link
Contributor

Hi @MovieStoreGuy I assume you must be busy with other things or perhaps not available at this time, could you at least suggest an alternative reviewer?

@ben-childs-docusign
Copy link
Contributor

bump, @MovieStoreGuy , @pavolloffay could we please finalize this PR?

@evan-bradley
Copy link
Contributor

@pavolloffay Could you take a look at this one?

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

LGTM

@ben-childs-docusign
Copy link
Contributor

Hi @evan-bradley , @pavolloffay - any chance we could get this one merged this week?

@MovieStoreGuy MovieStoreGuy merged commit 821c256 into open-telemetry:main Jul 23, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants