More Transaction Metrics #4781
More Transaction Metrics #4781
Conversation
Added more transaction metrics, and benchmark tests for transaction throttling.
| private readonly TransactionRateLoadSheddingOptions options; | ||
| private readonly PeriodicAction monitor; | ||
| private long transactionStartedAtLastCheck; | ||
| private TransactionAgentStatistics lastStatistics; |
There was a problem hiding this comment.
this should be ITransactionAgentStatistic too?
| Interlocked.Increment(ref this.transactionsThrottled); | ||
| } | ||
|
|
||
| public static TransactionAgentStatistics Create(ITransactionAgentStatistics initialStatistics) |
There was a problem hiding this comment.
why isn't this return a ITransactionAgentStatistics as well?
There was a problem hiding this comment.
This method seems more like a copy method than a create method
| @@ -66,8 +64,7 @@ public bool IsOverloaded() | |||
|
|
|||
| DateTime now = DateTime.UtcNow; | |||
| this.monitor.TryAction(now); | |||
| long startCounter = this.statistics.TransactionStartedCounter; | |||
| double txPerSecondCurrently = CalculateTps(this.transactionStartedAtLastCheck, this.lastCheckTime, startCounter, now); | |||
| double txPerSecondCurrently = CalculateTps(this.lastStatistics.TransactionsStarted, this.lastCheckTime, this.statistics.TransactionsStarted, now); | |||
There was a problem hiding this comment.
where do you track ITransactionAgentStatistic.Throttled ? did I miss anything. I thought it will be tracked here
There was a problem hiding this comment.
It's tracked in the transaction agent, where the IsOverloaded is called. The agent is responsible for updating it's statistics.
| { | ||
| return CommitReadOnlyTransaction(transactionInfo); | ||
| } | ||
| TransactionalStatus status = (writeParticipants == null) |
There was a problem hiding this comment.
Does there need a try/catch to ensure we track failed txns correctly, or are these async methods 'guaranteed' to not throw?
| private const string FailedTransactionsPerSecondMetric = "TransactionAgent.FailedTransactions_PerSecond"; | ||
|
|
||
| private const string ThrottledTransactionsTotalMetric = "TransactionAgent.ThrottledTransactions_Total"; | ||
| private const string ThrottledTransactionsPerSecondMetric = "TransactionAgent.ThrottledTransactions_PerSecond"; |
There was a problem hiding this comment.
Convention is to use only . as separators - see StatisticNames.cs
| var currentReported = TransactionAgentStatistics.Create(statistics); | ||
|
|
||
| this.telemetryProducer.TrackMetric(TransactionsStartedTotalMetric, currentReported.TransactionsStarted); | ||
| this.telemetryProducer.TrackMetric(TransactionsStartedPerSecondMetric, PerSecond(this.lastReported.TransactionsStarted, currentReported.TransactionsStarted, now - lastReportTime)); |
There was a problem hiding this comment.
Could extract var reportPeriod = now - lastReportTime
| @@ -13,25 +13,24 @@ public static class SiloBuilderExtensions | |||
| /// <summary> | |||
| /// Configure cluster to use the distributed TM algorithm | |||
| /// </summary> | |||
| public static ISiloHostBuilder UseDistributedTM(this ISiloHostBuilder builder) | |||
| public static ISiloHostBuilder UseDistributedTM(this ISiloHostBuilder builder, bool withReporter = true) | |||
There was a problem hiding this comment.
This new parameter is not obvious and hence could benefit from a doc comment.
Slightly prefer naming it reportStatistics.
I'm not entirely convinced that this needs to be exposed - or could we implement this using the options pattern somehow?
There was a problem hiding this comment.
Issue here is that users can provide their own statistics reporting. So they can configure tm with reporting, or without, and provide their own. I'll add a comment.
As for being part of the options, the Tm has no options now and this is really about what we're setting up in the container, not how the TM is configured. :/
|
@dotnet-bot test functional please |
|
I'll link as tangential to #4805 (comment) since it appears I just linked an example doing a bit of the same. |
|
@dotnet-bot test functional please |
Added more transaction metrics, and benchmark tests for transaction throttling.