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

[Kafka output] removed regex validation to allow dynamic topic #40415

Merged
merged 11 commits into from
Aug 14, 2024

Conversation

juliaElastic
Copy link
Contributor

@juliaElastic juliaElastic commented Aug 1, 2024

Proposed commit message

Removed regex validation to allow dynamic topic.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

  • build beats, copy over agentbeat to the elastic-agent build
# build beats agentbeat
cd ~/beats/x-pack/agentbeat 
SNAPSHOT=true PLATFORMS="darwin/amd64" mage package

# build agent
cd ~/elastic-agent
DEV=true EXTERNAL=false SNAPSHOT=true PLATFORMS=darwin/amd64 PACKAGES=tar.gz mage -v package

# copy agentbeat
cp ~/beats/x-pack/agentbeat/build/golang-crossbuild/agentbeat-darwin-amd64 ~/elastic-agent/build/distributions/elastic-agent-8.16.0-SNAPSHOT-darwin-x86_64/data/elastic-agent-3f22cc/components/agentbeat

To test with a real kafka server, follow this guide: https://hevodata.com/learn/install-kafka-on-mac/#Step_2_Download_Install_Kafka_on_Mac_Manually_or_via_HomeBrew

  • downloaded and started zookeeper and kafka server
  • created a topic system.cpu and started the script to consume messages
cd ~/Downloads/kafka_2.13-3.8.0
./bin/zookeeper-server-start.sh config/zookeeper.properties
./bin/kafka-server-start.sh config/server.properties
./bin/kafka-topics.sh --create --bootstrap-server localhost:9092 --replication-factor 1 --partitions 1 --topic system.cpu
./bin/kafka-console-consumer.sh --bootstrap-server localhost:9092 --topic system.cpu
  • added kafka output by entering host localhost:9092 and topic %{[data_stream.dataset]}
  • after enrolling the agent, see messages showing up in the kafka consumer
  • in agent logs, expect no errors
image image image

Related issues

@juliaElastic juliaElastic self-assigned this Aug 1, 2024
@juliaElastic juliaElastic requested a review from a team as a code owner August 1, 2024 13:00
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 1, 2024
Copy link
Contributor

mergify bot commented Aug 1, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @juliaElastic? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@juliaElastic juliaElastic added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Aug 1, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 1, 2024
@juliaElastic juliaElastic added the backport-skip Skip notification from the automated backport with mergify label Aug 1, 2024
@pierrehilbert pierrehilbert requested review from faec and removed request for VihasMakwana August 1, 2024 13:17
@faec
Copy link
Contributor

faec commented Aug 1, 2024

This check was added intentionally to block dynamic topics in Agent, since they can't be carried over in the OTel migration. Has this decision changed? The code looks fine but last I knew there was a business decision not to support it.

@juliaElastic
Copy link
Contributor Author

juliaElastic commented Aug 2, 2024

This check was added intentionally to block dynamic topics in Agent, since they can't be carried over in the OTel migration. Has this decision changed? The code looks fine but last I knew there was a business decision not to support it.

We decided to add it back dynamic topics support.
Perhaps we could make the validation stricter to only allow a specific field %{[FIELD]}, not any format.

Copy link
Contributor Author

@juliaElastic juliaElastic Aug 2, 2024

Choose a reason for hiding this comment

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

trying to undo the changes added here: https://github.com/elastic/beats/pull/37902/files#diff-d8e7a8c5739c80fb30e5c2f22ff87b2c8d476d8e9b71c222f2bcd6b4a6d0c3d6
though the dynamic topic resolution still doesn't seem to work

tested with standalone agent, but the topic resolution doesn't seem to work:

{"log.level":"error","@timestamp":"2024-08-02T12:01:28.385Z","message":"Kafka (topic=%{[data_stream.dataset]}): kafka server: The request attempted to perform an operation on an invalid topic.","component":{"binary":"filebeat","dataset":"elastic_agent.filebeat","id":"log-e1d038f6-44ea-4a4e-bd8c-e03fc4b224ac","type":"log"},"log":{"source":"log-e1d038f6-44ea-4a4e-bd8c-e03fc4b224ac"},"log.logger":"kafka","log.origin":{"file.line":338,"file.name":"kafka/client.go","function":"github.com/elastic/beats/v7/libbeat/outputs/kafka.(*client).errorWorker"},"service.name":"filebeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmacknz Can I get some help/review on this? I'm not sure why the dynamic topic is not being resolved in agent. Tried to add debug logs locally, but I'm not seeing the buildTopicSelector function being triggered.

Copy link
Member

@cmacknz cmacknz Aug 6, 2024

Choose a reason for hiding this comment

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

Hmm, I see some things to adjust but nothing that would cause that function not to be hit at all.

How are you testing this? You now need to build x-pack/agentbeat and copy over the resulting agentbeat binary, so make sure you are doing that and not copying one of the beat binaries directly.

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 didn't copy anything, I thought the EXTERNAL=false flag should build and use the local beats repo. I'll try copying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay I could build beats, copied over to agent and now I can see the dynamic topic resolution works as expected, I'll update the description with screenshots

Copy link
Member

Choose a reason for hiding this comment

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

Probably EXTERNAL=false isn't updated to be aware of agentbeat yet.

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 think I missed the part where the beats are not rebuilt if it was built already, and the step to copy it over to agent.

Copy link
Contributor

mergify bot commented Aug 9, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b kafka-dynamic-topic upstream/kafka-dynamic-topic
git merge upstream/main
git push upstream kafka-dynamic-topic

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Tiago Queiroz <[email protected]>
@juliaElastic
Copy link
Contributor Author

@belimawr @cmacknz Are you ready to approve? I would like to get this in before the sprint ends this week.

@juliaElastic juliaElastic merged commit 5efae2b into elastic:main Aug 14, 2024
122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants