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

x-pack/filebeat/input/azureeventhub: Support partitionID in Azure eventhub input #36948

Closed
wants to merge 6 commits into from

Conversation

bhapas
Copy link
Contributor

@bhapas bhapas commented Oct 24, 2023

Proposed commit message

See Title

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.

Related issues

@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @bhapas? 🙏.
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

@bhapas bhapas force-pushed the azure-event-hub-partition-id branch from 6d42ed2 to 172a724 Compare October 24, 2023 08:03
@bhapas bhapas added the backport-skip Skip notification from the automated backport with mergify label Oct 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 2023

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 azure-event-hub-partition-id upstream/azure-event-hub-partition-id
git merge upstream/main
git push upstream azure-event-hub-partition-id

@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 24, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-10-26T07:39:56.933+0000

  • Duration: 73 min 36 sec

Test stats 🧪

Test Results
Failed 0
Passed 3231
Skipped 176
Total 3407

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@Martin-Kemp
Copy link

Martin-Kemp commented Oct 26, 2023

Hi @bhapas, I pulled this locally to test and it seems to fail with a segmentation violation:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x5574e0828827]

goroutine 154 [running]:
github.com/elastic/beats/v7/x-pack/filebeat/input/azureeventhub.(*azureInput).runWithEPH.func1({0x0?, 0xc000311620?}, 0x0?)
        github.com/elastic/beats/v7/x-pack/filebeat/input/azureeventhub/eph.go:77 +0x27
github.com/Azure/azure-event-hubs-go/v3/eph.(*EventProcessorHost).compositeHandlers.func1.1(0x1c?)
        github.com/Azure/azure-event-hubs-go/[email protected]/eph/eph.go:445 +0x7c
created by github.com/Azure/azure-event-hubs-go/v3/eph.(*EventProcessorHost).compositeHandlers.func1
        github.com/Azure/azure-event-hubs-go/[email protected]/eph/eph.go:443 +0x1f6

So maybe I was wrong, and partionID is still not supported.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 26, 2023

@Martin-Kemp Microsoft recommends to migrate to use new library https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/messaging/azeventhubs/README.md.

Not sure if they added support in older library? The Migration Guide seems to impact a lot of other places. So probably need to raise another issue to tackle this first and then come back to check/fix PartitionID part

@Martin-Kemp
Copy link

Ah okay thanks @bhapas, I was hoping there's an easy fix for partitionID.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 26, 2023

@Martin-Kemp We can still look at the Event struct if they are writing the PartitionKey on the Event that comes in. I can add a log line and can you test again?

@Martin-Kemp
Copy link

@Martin-Kemp We can still look at the Event struct if they are writing the PartitionKey on the Event that comes in. I can add a log line and can you test again?

Sure, I'll be happy to test it.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 26, 2023

@Martin-Kemp Just pushed a commit and updated the base as well. Can you try testing these log lines

@Martin-Kemp
Copy link

There's also the method GetPartitionInformation on a hub object: https://github.com/Azure/azure-event-hubs-go/blob/07e5a11a61336dfb2c5d6276c0298bb4e2d38625/hub.go#L524

Which could be useful. But doesn't look like eph uses this object.

@zmoog
Copy link
Contributor

zmoog commented Oct 26, 2023

@bhapas, there's an issue tracking the need to upgrade to the new Event Hub SDK; it is waiting for a team to prioritize it.

I found the partitionID part already commented out when I joined Elastic and started working on the Azure ecosystem, so I don't know the backstory here. I tried getting the partitionID in the past, but the old SDK does not support this metadata reliably.

The new SDK brings significant changes to the Event Hub; it replaces EPH with another non-compatible event hub client.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 26, 2023

@bhapas, there's an issue tracking the need to upgrade to the new Event Hub SDK; it is waiting for a team to prioritize it.

I found the partitionID part already commented out when I joined Elastic and started working on the Azure ecosystem, so I don't know the backstory here. I tried getting the partitionID in the past, but the old SDK does not support this metadata reliably.

The new SDK brings significant changes to the Event Hub; it replaces EPH with another non-compatible API.

@zmoog That probably answers why it is behaving so. But should the commented out line point to this issue so that it is easy for users to track it and not get lost with this?

@zmoog
Copy link
Contributor

zmoog commented Oct 26, 2023

That probably answers why it is behaving so. But should the commented out line point to this issue so that it is easy for users to track it and not get lost with this?

We can check if we're running the latest minor/patch release of the old SDK and try it. I'm happy to help debugging with the input to see if we get the partitionID in the callback from EPH.

If we fail, we can add a comment pointing to the issue tracking the SDK upgrade and a +1 on the SDK upgrade.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 26, 2023

@zmoog Feel free to take over this PR and update to the latest release and run this in debug. Interested to see if Microsoft maintained this in old SDK.

@Martin-Kemp
Copy link

@Martin-Kemp Just pushed a commit and updated the base as well. Can you try testing these log lines

This also crashes while trying to log:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x55b3c24f4db2]

goroutine 133 [running]:
github.com/elastic/beats/v7/x-pack/filebeat/input/azureeventhub.(*azureInput).runWithEPH.func1({0x0?, 0x0?}, 0xc001231400)
        github.com/elastic/beats/v7/x-pack/filebeat/input/azureeventhub/eph.go:78 +0x52
github.com/Azure/azure-event-hubs-go/v3/eph.(*EventProcessorHost).compositeHandlers.func1.1(0xc001a7e900?)
        github.com/Azure/azure-event-hubs-go/[email protected]/eph/eph.go:441 +0x7c
created by github.com/Azure/azure-event-hubs-go/v3/eph.(*EventProcessorHost).compositeHandlers.func1
        github.com/Azure/azure-event-hubs-go/[email protected]/eph/eph.go:439 +0x1f6

@Martin-Kemp
Copy link

@zmoog @bhapas let me know if you need me to test anything.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 26, 2023

@Martin-Kemp Azure/azure-event-hubs-go#274 might be talking about what we are trying here and it seems the new SDK client supports this

@zmoog
Copy link
Contributor

zmoog commented Oct 26, 2023

Running the input in debug mode shows that both e.PartitionKey and e.SystemProperties.PartitionKey are nil for some non-obvious reasons:

CleanShot 2023-10-26 at 15 48 42@2x

I'll bump the old SDK to the latest version and try again.

@zmoog
Copy link
Contributor

zmoog commented Oct 26, 2023

Unfortunately, after upgrading the current/old SDK to v3.6.1, I got the same result.

go get github.com/Azure/azure-event-hubs-go/[email protected]   

CleanShot 2023-10-26 at 16 10 23@2x

@Martin-Kemp
Copy link

Martin-Kemp commented Oct 26, 2023

Thanks for all the effort @zmoog and @bhapas.

@zmoog
Copy link
Contributor

zmoog commented Oct 27, 2023

@Martin-Kemp, thank you for upvoting the SDK upgrade by commenting on the tracking issue!

I'll bring the attention to the need to do this SDK upgrade.

@bhapas
Copy link
Contributor Author

bhapas commented Oct 27, 2023

Closing this PR as this is not relevant anymore.

@bhapas bhapas closed this Oct 27, 2023
@Martin-Kemp
Copy link

I can confirm that the new SDK works. I tested this basic example: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/messaging/azeventhubs/example_consuming_events_test.go
and followed this documentation: https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/messaging/azeventhubs#section-readme

I sent a message to an eventhub and specified the partitionKey then read the message and got the partitionKey:

Received message: Your message content, PartitionKey: %!d(string=your-partition-key), sequence number: 0, enqueued time: 2023-10-27 10:34:41.948 +0000 UTC

By the way, it turns out I was looking for PartitionKey, not PartitionID. But both should be available with the new SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.12-candidate backport-skip Skip notification from the automated backport with mergify enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support partitionID in Azure eventhub input
4 participants