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

**DRAFT** feat: Azure Service Bus Instrumentation #2880

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

tippmar-nr
Copy link
Member

@tippmar-nr tippmar-nr commented Nov 11, 2024

Thank you for submitting a pull request. Please review our contributing guidelines and code of conduct.

Description

This PR adds instrumentation for Azure Service Bus, for applications that use the Azure.Messaging.ServiceBus NuGet package, v7.12.0 and later.

The following methods are instrumented:

  • ServiceBusReceiver:
    • ReceiveMessageAsync() and ReceiveMessagesAsync()
    • ReceiveDeferredMessageAsync() and ReceiveDeferredMessagesAsync()
    • PeekMessageAsync() and PeekMessagesAsync()
    • CompleteMessageAsync()
    • AbandonMessageAsync()
    • DeadLetterMessageAsync()
    • DeferMessageAsync()
    • RenewMessageLockAsync()
  • ServiceBusSender:
    • SendMessageAsync() and SendMessagesAsync()
    • ScheduleMessageAsync() and ScheduleMessagesAsync()
    • CancelScheduledMessageAsync() and CancelScheduledMessagesAsync()

Distributed tracing support is included, using the ApplicationProperties dictionary.

Author Checklist

  • Unit tests, Integration tests, and Unbounded tests completed
  • Performance testing completed with satisfactory results (if required)

Reviewer Checklist

  • Perform code review
  • Pull request was adequately tested (new/existing tests, performance tests)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.45%. Comparing base (d1e29ea) to head (81f48ae).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2880      +/-   ##
==========================================
+ Coverage   81.43%   81.45%   +0.02%     
==========================================
  Files         465      465              
  Lines       29573    29573              
  Branches     3275     3275              
==========================================
+ Hits        24084    24090       +6     
+ Misses       4692     4688       -4     
+ Partials      797      795       -2     
Flag Coverage Δ
Agent 82.39% <ø> (+0.02%) ⬆️
Profiler 73.07% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

{
"SendMessagesAsync" => MessageBrokerAction.Produce,
"ScheduleMessagesAsync" => MessageBrokerAction.Produce,
"CancelScheduledMessagesAsync" => MessageBrokerAction.Purge, // TODO is this correct ???
Copy link
Member

Choose a reason for hiding this comment

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

This seems correct based on the description of the method.

{
protected const string BrokerVendorName = "AzureServiceBus";

public bool IsTransactionRequired => true; // only instrument service bus methods if we're already in a transaction
Copy link
Member

Choose a reason for hiding this comment

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

This seems different compared with other MQ that we instrument. Shouldn't the receive generate a transaction?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably a topic for pairing on Tuesday. But this implementation is consistent with our MSMQ instrumentation. Unlike RabbitMQ, there's not an "eventing" layer that we could instrument that would wrap processing time on the client application.

Copy link
Member Author

@tippmar-nr tippmar-nr Nov 11, 2024

Choose a reason for hiding this comment

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

Looks like we could potentially instrument ServiceBusProcessor which does implement an event-based receive model where client code is called when a message is received. I'll take a quick look to see if it's worth the effort or not.

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.

3 participants