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

Add better OTel information into Brighter #3096

Draft
wants to merge 67 commits into
base: master
Choose a base branch
from
Draft

Conversation

iancooper
Copy link
Member

We created semantic conventions for Brighter: https://github.com/BrighterCommand/Brighter/blob/master/docs/adr/0010-brighter-semantic-conventions.md

This PR is about adding those conventions into Brighter.

@iancooper iancooper added 2 - In Progress feature request v10 Allocal to a v10 release .NET Pull requests that update .net code labels May 17, 2024
@iancooper iancooper self-assigned this May 17, 2024
try
{
var successfullySentMessage = new List<string>();

foreach (var batch in SplitRequestBatchIntoTypes(requests))
foreach (var request in requests)
Copy link
Member

Choose a reason for hiding this comment

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

This is a significant change in Functionality, Previously we split into types (so that we could pass through the correct request type and the provider factory could correctly identity the request type) and then put them as a bulk operation onto the bus as well as marking them as disposed as a single Bulk Operation. This will sent them to the bus one by one and then dispatch them one by one

Copy link
Member Author

@iancooper iancooper May 28, 2024

Choose a reason for hiding this comment

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

So the binding to the type happens now in the call to DepositPost. This means that the DepositPost has the correct request type and can be identified by the mapper factory correctly.

Were we batching up writes to the Outbox i.e. using a bulk sql insert? We tend to pay the cost for the pipeline transform call per message. Just trying to figure out if it is batching to the Outbox that we need to fix, over other parts.

Batch Clear is a separate issue, as it does not involve the message transform pipeline, so we don't have the same issues.

It is worth noting the following:

* The `BulkMessageMap` is a bulk, iterative operation; it's main purpose is late-binding of the type from an `IEnumerable<IRequest>`
* Producing a `Message` via a `Producer` is not a bulk operation in Brighter
Copy link
Member

Choose a reason for hiding this comment

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

Brighter v9 does support Bulk clearing via the IAmABulkMessageProducerAsync interface

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look, although we have not made changes to clear along these lines yet

* The `BulkMessageMap` is a bulk, iterative operation; it's main purpose is late-binding of the type from an `IEnumerable<IRequest>`
* Producing a `Message` via a `Producer` is not a bulk operation in Brighter

The only bulk operations we have in the flow are the read and write from the `Outbox`
Copy link
Member

Choose a reason for hiding this comment

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

In v9 there is also a Bulk Mark Dispatches via the MarkDispatchedAsync(IEnumerable ids, ...) method on the relational outbox

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, we won't have changed that

@preardon
Copy link
Member

preardon commented Jun 1, 2024

@iancooper Talking about bulk I see a couple of Paths here, Generally I use implicit clearing of the outbox so in my application code I am only ever doing a Bulk outbox add (where necessary). The most important flow in my mind is the Bulk dispatch of messages, at the moment if we consider a batch of 100 messages that are to be dispatched:

Without Bulk Dispatch

  • Call CommandProcessor.ClearOutbox
  • Get Messages to Dispatch
    • For Each Message Id
      • Open SQL Connection
      • Run Query
      • Close SQL Connection
  • For each Message
    • Publish Message
    • Mark As Dispatched
      • Open SQL Connection
      • Run SQL Command
      • Close Connection

In this instance we have

  • 200 SQL Connections Opened and Closed
  • 100 call to the Provider

This is very slow, in my experience dispatching from the outbox is slower than consuming (from memory it was around a message or two a second) so no active messages end up on the queue

The same example above with Bulk (as is currently functioning in v9 for MsSql and ASB)

  • Call CommandProcessor.ClearOutbox
  • Get Messages to Dispatch
    • Open SQL Connection
    • Run Query
    • Close SQL Connection
  • Publish Messages
    • Mark As Dispatched
      • Open SQL Connection
      • Run SQL Command
      • Close Connection

In this instance we have

  • 2 SQL Connections Opened and Closed
  • 1 call to the Provider

Using this method I have successfully been able to dispatch a very large weight of messages (>100k) in well under a minute.

Without the ability to bulk dispatch my system will not work (we dispatch millions of messages per day)

@iancooper
Copy link
Member Author

@iancooper The most important flow in my mind is the Bulk dispatch of messages, at the moment if we consider a batch of 100 messages that are to be dispatched:

Not altered here. I'll pick up batches after. It would be good to support both batch writes to the outbox and batch read and write to producer.

There is a V10 change because we need the publication for the pipeline, so that we can use it for CloudEvents (and its there for generic mappers) so we do publication lookup when we run the transform/mapping pipeline.

That means we would need to do something a little different for deposit post as I moved the late binding earlier.

But the clear, I am still looking at

@preardon
Copy link
Member

preardon commented Jun 1, 2024

@iancooper The most important flow in my mind is the Bulk dispatch of messages, at the moment if we consider a batch of 100 messages that are to be dispatched:

Not altered here. I'll pick up batches after. It would be good to support both batch writes to the outbox and batch read and write to producer.

There is a V10 change because we need the publication for the pipeline, so that we can use it for CloudEvents (and its there for generic mappers) so we do publication lookup when we run the transform/mapping pipeline.

That means we would need to do something a little different for deposit post as I moved the late binding earlier.

But the clear, I am still looking at

Excellent, as long as in the Deposit Flow we can do a single Command for insert this will be excellent! let me know if there are any parts you want me to look at

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress feature request .NET Pull requests that update .net code v10 Allocal to a v10 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants