Archiver Observability #3353
Merged
Archiver Observability #3353
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… first, ahead of oberservability
…the service bus. Presumably **your** archiver would have to do the same.
iancooper
added
2 - In Progress
feature request
v10
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
| @@ -41,6 +41,7 @@ public static class BrighterSpanExtensions | |||
| CommandProcessorSpanOperation.Publish => "publish", | |||
| CommandProcessorSpanOperation.Send => "send", | |||
| CommandProcessorSpanOperation.Clear => "clear", | |||
| CommandProcessorSpanOperation.Archive => "archive", | |||
There was a problem hiding this comment.
❌ New issue: Complex Method
ToSpanName has a cyclomatic complexity of 9, threshold = 9
Comment on lines
+287
to
+292
|
|
||
| var span = Tracer?.CreateDbSpan( | ||
| new OutboxSpanInfo(DbSystem.Brighter, InMemoryAttributes.DbName, OutboxDbOperation.DispatchedMessages, InMemoryAttributes.DbTable), | ||
| requestContext?.Span, | ||
| options: _instrumentationOptions | ||
| ); |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 4 functions with similar structure: DispatchedMessages,Get,MarkDispatched,OutstandingMessages
| @@ -214,6 +214,8 @@ public Task AddAsync( | |||
| IAmABoxTransactionProvider<CommittableTransaction>? transactionProvider = null, | |||
| CancellationToken cancellationToken = default) | |||
| { | |||
| //NOTE: As we call Add, don't create telemetry here | |||
There was a problem hiding this comment.
✅ Getting better: Missing Arguments Abstractions
The average number of function arguments decreases from 4.63 to 4.47, threshold = 4.00
| @@ -214,6 +214,8 @@ public Task AddAsync( | |||
| IAmABoxTransactionProvider<CommittableTransaction>? transactionProvider = null, | |||
| CancellationToken cancellationToken = default) | |||
| { | |||
| //NOTE: As we call Add, don't create telemetry here | |||
There was a problem hiding this comment.
❌ New issue: Primitive Obsession
In this module, 30.3% of all function arguments are primitive types, threshold = 30.0%
| @@ -121,15 +121,14 @@ public void When_Clearing_A_Message_A_Span_Is_Exported() | |||
| _traceProvider.ForceFlush(); | |||
|
|
|||
| //assert | |||
| _exportedActivities.Count.Should().Be(7); | |||
| _exportedActivities.Count.Should().Be(8); | |||
There was a problem hiding this comment.
❌ Getting worse: Complex Method
When_Clearing_A_Message_A_Span_Is_Exported increases in cyclomatic complexity from 45 to 49, threshold = 9
| @@ -121,7 +121,7 @@ public async Task When_Clearing_A_Message_A_Span_Is_Exported() | |||
| _traceProvider.ForceFlush(); | |||
|
|
|||
| //assert | |||
| _exportedActivities.Count.Should().Be(7); | |||
| _exportedActivities.Count.Should().Be(8); | |||
There was a problem hiding this comment.
❌ Getting worse: Complex Method
When_Clearing_A_Message_A_Span_Is_Exported increases in cyclomatic complexity from 45 to 49, threshold = 9
iancooper
requested review from
holytshirt,
DevJonny and
preardon
as code owners
Comment on lines
+90
to
+158
| [Fact] | ||
| public void When_archiving_from_the_outbox() | ||
| { | ||
| var parentActivity = new ActivitySource("Paramore.Brighter.Tests").StartActivity("BrighterTracerSpanTests"); | ||
|
|
||
| var context = new RequestContext(); | ||
| context.Span = parentActivity; | ||
|
|
||
| //add and clear message | ||
| var myEvent = new MyEvent(); | ||
| var myMessage = new MyEventMessageMapper().MapToMessage(myEvent, _publication); | ||
| _bus.AddToOutbox(myMessage, context); | ||
| _bus.ClearOutbox([myMessage.Id], context); | ||
|
|
||
| //se should have an entry in the outbox | ||
| _outbox.EntryCount.Should().Be(1); | ||
|
|
||
| //allow time to pass | ||
| _timeProvider.Advance(TimeSpan.FromSeconds(300)); | ||
|
|
||
| //archive | ||
| var dispatchedSince = TimeSpan.FromSeconds(100); | ||
| _archiver.Archive(dispatchedSince, context); | ||
|
|
||
| //should be no messages in the outbox | ||
| _outbox.EntryCount.Should().Be(0); | ||
|
|
||
| parentActivity?.Stop(); | ||
|
|
||
| _traceProvider.ForceFlush(); | ||
|
|
||
| //We should have exported matching activities | ||
| _exportedActivities.Count.Should().Be(9); | ||
|
|
||
| _exportedActivities.Any(a => a.Source.Name == "Paramore.Brighter").Should().BeTrue(); | ||
|
|
||
| //there should be a n archive create span for the batch | ||
| var createActivity = _exportedActivities.Single(a => a.DisplayName == $"{BrighterSemanticConventions.ArchiveMessages} {CommandProcessorSpanOperation.Archive.ToSpanName()}"); | ||
| createActivity.Should().NotBeNull(); | ||
| createActivity.ParentId.Should().Be(parentActivity?.Id); | ||
|
|
||
| //check for outstanding messages span | ||
| var osCheckActivity = _exportedActivities.SingleOrDefault(a => | ||
| a.DisplayName == $"{OutboxDbOperation.DispatchedMessages.ToSpanName()} {InMemoryAttributes.DbName} {InMemoryAttributes.DbTable}"); | ||
| osCheckActivity.Should().NotBeNull(); | ||
| osCheckActivity?.ParentId.Should().Be(createActivity.Id); | ||
|
|
||
| //check for delete messages span | ||
| var deleteActivity = _exportedActivities.SingleOrDefault(a => | ||
| a.DisplayName == $"{OutboxDbOperation.Delete.ToSpanName()} {InMemoryAttributes.DbName} {InMemoryAttributes.DbTable}"); | ||
| deleteActivity?.Should().NotBeNull(); | ||
| deleteActivity?.ParentId.Should().Be(createActivity.Id); | ||
|
|
||
| //check the tags for the create span | ||
| createActivity.TagObjects.Should().Contain(t => t.Key == BrighterSemanticConventions.ArchiveAge && Math.Abs(Convert.ToDouble(t.Value) - dispatchedSince.TotalMilliseconds) < TOLERANCE); | ||
|
|
||
| //check the tags for the outstanding messages span | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbOperation && t.Value == OutboxDbOperation.DispatchedMessages.ToSpanName()).Should().BeTrue(); | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbTable && t.Value == InMemoryAttributes.DbTable).Should().BeTrue(); | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbSystem && t.Value == DbSystem.Brighter.ToDbName()).Should().BeTrue(); | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbName && t.Value == InMemoryAttributes.DbName).Should().BeTrue(); | ||
|
|
||
| //check the tages for the delete messages span | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbOperation && t.Value == OutboxDbOperation.Delete.ToSpanName()).Should().BeTrue(); | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbTable && t.Value == InMemoryAttributes.DbTable).Should().BeTrue(); | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbSystem && t.Value == DbSystem.Brighter.ToDbName()).Should().BeTrue(); | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbName && t.Value == InMemoryAttributes.DbName).Should().BeTrue(); | ||
|
|
||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
When_archiving_from_the_outbox has a cyclomatic complexity of 10, threshold = 9
Comment on lines
+56
to
+72
| public OutboxArchiver( | ||
| IAmAnOutbox outbox, | ||
| IAmAnArchiveProvider archiveProvider, | ||
| IAmARequestContextFactory? requestContextFactory = null, | ||
| int archiveBatchSize = 100, | ||
| IAmABrighterTracer? tracer = null, | ||
| InstrumentationOptions instrumentationOptions = InstrumentationOptions.All) | ||
| { | ||
| _archiveProvider = archiveProvider; | ||
| _archiveBatchSize = archiveBatchSize; | ||
| _tracer = tracer; | ||
| _instrumentationOptions = instrumentationOptions; | ||
| _requestContextFactory = requestContextFactory ?? new InMemoryRequestContextFactory(); | ||
|
|
||
| if (outbox is IAmAnOutboxSync<TMessage, TTransaction> syncOutbox) _outBox = syncOutbox; | ||
| if (outbox is IAmAnOutboxAsync<TMessage, TTransaction> asyncOutbox) _asyncOutbox = asyncOutbox; | ||
| } |
There was a problem hiding this comment.
❌ New issue: Constructor Over-Injection
OutboxArchiver has 6 arguments, threshold = 5
| @@ -993,10 +1028,10 @@ public void ClearOutstandingFromOutbox( | |||
| var subscription = _replySubscriptions?.FirstOrDefault(s => s.DataType == typeof(TResponse)); | |||
|
|
|||
| if (subscription is null) | |||
| throw new ArgumentOutOfRangeException($"No Subscription registered fpr replies of type {typeof(T)}"); | |||
There was a problem hiding this comment.
❌ Getting worse: Code Duplication
introduced similar code in: ClearOutstandingFromOutbox
Comment on lines
+92
to
+160
| [Fact] | ||
| public async Task When_archiving_from_the_outbox() | ||
| { | ||
| var parentActivity = new ActivitySource("Paramore.Brighter.Tests").StartActivity("BrighterTracerSpanTests"); | ||
|
|
||
| var context = new RequestContext(); | ||
| context.Span = parentActivity; | ||
|
|
||
| //add and clear message | ||
| var myEvent = new MyEvent(); | ||
| var myMessage = new MyEventMessageMapper().MapToMessage(myEvent, _publication); | ||
| await _bus.AddToOutboxAsync(myMessage, context); | ||
| await _bus.ClearOutboxAsync([myMessage.Id], context); | ||
|
|
||
| //se should have an entry in the outbox | ||
| _outbox.EntryCount.Should().Be(1); | ||
|
|
||
| //allow time to pass | ||
| _timeProvider.Advance(TimeSpan.FromSeconds(300)); | ||
|
|
||
| //archive | ||
| var dispatchedSince = TimeSpan.FromSeconds(100); | ||
| await _archiver.ArchiveAsync(dispatchedSince, context); | ||
|
|
||
| //should be no messages in the outbox | ||
| _outbox.EntryCount.Should().Be(0); | ||
|
|
||
| parentActivity?.Stop(); | ||
|
|
||
| _traceProvider.ForceFlush(); | ||
|
|
||
| //We should have exported matching activities | ||
| _exportedActivities.Count.Should().Be(9); | ||
|
|
||
| _exportedActivities.Any(a => a.Source.Name == "Paramore.Brighter").Should().BeTrue(); | ||
|
|
||
| //there should be a n archive create span for the batch | ||
| var createActivity = _exportedActivities.Single(a => a.DisplayName == $"{BrighterSemanticConventions.ArchiveMessages} {CommandProcessorSpanOperation.Archive.ToSpanName()}"); | ||
| createActivity.Should().NotBeNull(); | ||
| createActivity.ParentId.Should().Be(parentActivity?.Id); | ||
|
|
||
| //check for outstanding messages span | ||
| var osCheckActivity = _exportedActivities.SingleOrDefault(a => | ||
| a.DisplayName == $"{OutboxDbOperation.DispatchedMessages.ToSpanName()} {InMemoryAttributes.DbName} {InMemoryAttributes.DbTable}"); | ||
| osCheckActivity.Should().NotBeNull(); | ||
| osCheckActivity?.ParentId.Should().Be(createActivity.Id); | ||
|
|
||
| //check for delete messages span | ||
| var deleteActivity = _exportedActivities.SingleOrDefault(a => | ||
| a.DisplayName == $"{OutboxDbOperation.Delete.ToSpanName()} {InMemoryAttributes.DbName} {InMemoryAttributes.DbTable}"); | ||
| deleteActivity.Should().NotBeNull(); | ||
| deleteActivity?.ParentId.Should().Be(createActivity.Id); | ||
|
|
||
| //check the tags for the create span | ||
| createActivity.TagObjects.Should().Contain(t => t.Key == BrighterSemanticConventions.ArchiveAge && Math.Abs(Convert.ToDouble(t.Value) - dispatchedSince.TotalMilliseconds) < TOLERANCE); | ||
|
|
||
| //check the tags for the outstanding messages span | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbOperation && t.Value == OutboxDbOperation.DispatchedMessages.ToSpanName()).Should().BeTrue(); | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbTable && t.Value == InMemoryAttributes.DbTable).Should().BeTrue(); | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbSystem && t.Value == DbSystem.Brighter.ToDbName()).Should().BeTrue(); | ||
| osCheckActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbName && t.Value == InMemoryAttributes.DbName).Should().BeTrue(); | ||
|
|
||
| //check the tages for the delete messages span | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbOperation && t.Value == OutboxDbOperation.Delete.ToSpanName()).Should().BeTrue(); | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbTable && t.Value == InMemoryAttributes.DbTable).Should().BeTrue(); | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbSystem && t.Value == DbSystem.Brighter.ToDbName()).Should().BeTrue(); | ||
| deleteActivity?.Tags.Any(t => t.Key == BrighterSemanticConventions.DbName && t.Value == InMemoryAttributes.DbName).Should().BeTrue(); | ||
|
|
||
| } |
There was a problem hiding this comment.
❌ New issue: Complex Method
When_archiving_from_the_outbox has a cyclomatic complexity of 10, threshold = 9
Comment on lines
+78
to
+79
| public OutboxProducerMediator( | ||
| IAmAProducerRegistry producerRegistry, |
There was a problem hiding this comment.
ℹ New issue: Complex Method
OutboxProducerMediator has a cyclomatic complexity of 14, threshold = 9
Comment on lines
+78
to
+79
| public OutboxProducerMediator( | ||
| IAmAProducerRegistry producerRegistry, |
There was a problem hiding this comment.
ℹ New issue: Constructor Over-Injection
OutboxProducerMediator has 14 arguments, threshold = 5
…due to additional parameters
| @@ -325,17 +321,17 @@ private static object BuildCommandProcessor(IServiceProvider provider) | |||
| return commandProcessor; | |||
| } | |||
|
|
|||
| private static IAmAnExternalBusService BuildExternalBus(IServiceProvider serviceProvider, | |||
There was a problem hiding this comment.
✅ No longer an issue: Excess Number of Function Arguments
BuildExternalBus is no longer above the threshold for number of arguments
| @@ -325,17 +321,17 @@ private static object BuildCommandProcessor(IServiceProvider provider) | |||
| return commandProcessor; | |||
| } | |||
|
|
|||
| private static IAmAnExternalBusService BuildExternalBus(IServiceProvider serviceProvider, | |||
| private static IAmAnOutboxProducerMediator BuildOutBoxProducerMediator(IServiceProvider serviceProvider, | |||
There was a problem hiding this comment.
ℹ New issue: Excess Number of Function Arguments
BuildOutBoxProducerMediator has 5 arguments, threshold = 4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want to add observability to the archiver.