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

Instrument event processor #7512

Merged
merged 9 commits into from
Sep 16, 2019

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Sep 6, 2019

Adds instrumentation for EventProcessor processing and checkpoints

Fixes: #7554

@pakrym pakrym requested review from jsquire and lmolkova September 6, 2019 19:33
Copy link
Member

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

left a couple of comments

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

There was a recent work stream for preview 3 that made some non-trivial changes to a lot of the Event Processor surface; you're likely to hit some conflicts, but I think the areas that you're in shouldn't have been too badly impacted.

I left a couple of notes to ask for Doc comments for internal consistency with the surrounding library code. If you're in a rush, let me know and I don't mind circling back to add them in.

@pakrym pakrym marked this pull request as ready for review September 9, 2019 22:33
@@ -6,6 +6,7 @@
<PackageReleaseNotes>https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/eventhub/Azure.Messaging.EventHubs/CHANGELOG.md</PackageReleaseNotes>
<TargetFrameworks>$(RequiredTargetFrameworks)</TargetFrameworks>
<EnableFxCopAnalyzers>false</EnableFxCopAnalyzers>
<UseProjectReferenceToAzureCore>true</UseProjectReferenceToAzureCore>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporary until we ship preview 3

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

Event Hubs has another package for preview 3 due to be released this week. If you wouldn't mind holding off on merging until that point, it would be appreciated.

Flipping to "Request Changes" to prevent accidental merge. Changes still LGTM.

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

Event Hubs is now unblocked. Thanks for your patience.

@pakrym pakrym merged commit 065dc77 into Azure:master Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrument EventHubs event processor for distribute tracing
3 participants