Feature - Ginger Telemetry #3897
Conversation
added MockTelemetryCollector
moved TelemetryRetryService
added feature tracking for POM learning
…n History to BPMN export
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request enhances telemetry tracking across several methods related to exporting BPMN data and API parsing. It introduces the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
Ginger/GingerCoreNET/Telemetry/Protos/V1/log.proto (1)
15-18: Consider removing theLogCountfield.The
AddLogsRequestmessage definition is well-structured for a request to add multiple log records.However, the
LogCountfield seems redundant as the count can be derived from the size of theLogslist. Consider removing this field to simplify the message definition.Apply this diff to remove the
LogCountfield:message AddLogsRequest { repeated LogRecord Logs = 1; - int32 LogCount = 2; }Ginger/GingerCoreNET/Telemetry/Protos/V1/feature.proto (2)
5-13: Consider using a map or a separate message type for theMetadatafield.The
Metadatafield is currently defined as a string, which might not be the most efficient or type-safe way to store arbitrary metadata. Consider using a map or a separate message type for structured metadata.For example, you could define a separate
FeatureMetadatamessage and use it as the type for theMetadatafield:message FeatureMetadata { map<string, string> Fields = 1; } message FeatureRecord { // ... FeatureMetadata Metadata = 7; }This allows for more structured and type-safe metadata storage.
15-18: Consider removing theFeatureCountfield.The
FeatureCountfield seems redundant, as the number of feature records can be determined from the size of theFeatureslist. Unless there is a specific reason to include this field, consider removing it to simplify the message definition.message AddFeaturesRequest { repeated FeatureRecord Features = 1; - int32 FeatureCount = 2; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Ginger/GingerCoreNET/GingerCoreNET.csproj (2 hunks)
- Ginger/GingerCoreNET/Telemetry/Protos/V1/feature.proto (1 hunks)
- Ginger/GingerCoreNET/Telemetry/Protos/V1/log.proto (1 hunks)
- Ginger/GingerCoreNET/Telemetry/TelemetryCollector.cs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Ginger/GingerCoreNET/GingerCoreNET.csproj
- Ginger/GingerCoreNET/Telemetry/TelemetryCollector.cs
Additional comments not posted (5)
Ginger/GingerCoreNET/Telemetry/Protos/V1/log.proto (3)
5-13: LGTM!The
LogRecordmessage definition is well-structured and captures relevant information for a log record.
20-22: LGTM!The
AddLogsResponsemessage definition is well-structured for a response to theAddLogsRequest.
24-26: LGTM!The
LogCollectorservice definition is well-structured for collecting log records.Ginger/GingerCoreNET/Telemetry/Protos/V1/feature.proto (2)
20-22: LGTM!The
AddFeaturesResponsemessage definition looks good. TheFeatureCountfield provides useful information about the number of feature records processed in the correspondingAddFeaturesRequest.
24-26: LGTM!The
FeatureCollectorservice definition and itsCollectmethod look good. The method provides a simple and clear interface for collecting feature records, using the appropriate request and response message types.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- Ginger/GingerCoreNET/Telemetry/BlockingBufferQueue.cs (1 hunks)
- Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs (1 hunks)
- Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs
Additional context used
GitHub Check: Codacy Static Code Analysis
Ginger/GingerCoreNET/Telemetry/BlockingBufferQueue.cs
[failure] 108-108: Ginger/GingerCoreNET/Telemetry/BlockingBufferQueue.cs#L108
Either implement 'IDisposable.Dispose', or totally rename this method to prevent confusion.
[failure] 110-110: Ginger/GingerCoreNET/Telemetry/BlockingBufferQueue.cs#L110
Add curly braces around the nested statement(s) in this 'if' block.Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs
[warning] 21-21: Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs#L21
A static field in a generic type is not shared among instances of different close constructed types.
[failure] 55-55: Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs#L55
Dispose '_cancellationTokenSource' when it is no longer needed.
Additional comments not posted (10)
Ginger/GingerCoreNET/Telemetry/BlockingBufferQueue.cs (4)
27-36: LGTM!The constructor correctly initializes all the required fields.
38-58: LGTM!The
Enqueuemethod correctly implements the enqueue operation with proper synchronization and error handling.
60-101: LGTM!The
Dequeuemethod correctly implements the dequeue operation with proper synchronization and error handling. The use of a cancellation token to break out of the loop is a good practice.
103-106: LGTM!The
Flushmethod correctly implements the flush operation by canceling the cancellation token source.Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs (6)
64-88: Handle exceptions thrown by the background tasks.The
CreateConsumerTaskandCreateRetryTaskmethods create background tasks that are not monitored for exceptions. If a task throws an exception, it will terminate silently. Consider usingTask.Runto create the tasks andtry-catchto handle exceptions.Also applies to: 90-121
96-105: Check for cancellation before polling for records.The
CreateRetryTaskmethod does not handle cancellation requests gracefully. It should check for cancellation before polling for records to avoid unnecessary work.Apply this diff to fix the issue:
-while (!_cancellationTokenSource.IsCancellationRequested) +while (true) { + if (_cancellationTokenSource.IsCancellationRequested) + { + break; + } + string corrId = NewCorrelationId();
173-180: Log a warning message when the collector returns a non-successful result.The
ProcessAsyncandReprocessAsyncmethods do not handle the case where the collector returns a non-successful result. They should log a warning message to alert the user.Apply this diff to fix the issue:
if (result != null && result.Successful) { await TryDeleteRecordsFromDBAsync(records, corrId); } else { + _logger?.LogWarning("{corrId} collector returned non-successful result in {queueName}", corrId, QueueName); await TryIncrementUploadAttemptCountAsync(records, corrId); }Also applies to: 199-206
76-83: Catch and log exceptions thrown by theProcessAsyncmethod.The
CreateConsumerTaskmethod does not handle exceptions thrown by theProcessAsyncmethod. It should catch and log exceptions to avoid terminating the task.Apply this diff to fix the issue:
try { await ProcessAsync(records, corrId); } +catch (Exception ex) +{ + _logger?.LogError("{corrId} error while processing records in {queueName}\n{ex}", corrId, QueueName, ex); +}
198-207: Catch and log exceptions thrown by theTrySendToCollectorAsyncandTryDeleteRecordsFromDBAsyncmethods.The
ReprocessAsyncmethod does not handle exceptions thrown by theTrySendToCollectorAsyncandTryDeleteRecordsFromDBAsyncmethods. It should catch and log exceptions to avoid terminating the method.Apply this diff to fix the issue:
+try +{ ITelemetryCollector<TRecord>.AddResult? result = await TrySendToCollectorAsync(records, corrId); +} +catch (Exception ex) +{ + _logger?.LogError("{corrId} error while sending records to collector in {queueName}\n{ex}", corrId, QueueName, ex); +} + +try +{ if (result != null && result.Successful) { await TryDeleteRecordsFromDBAsync(records, corrId); } } +catch (Exception ex) +{ + _logger?.LogError("{corrId} error while deleting records from db in {queueName}\n{ex}", corrId, QueueName, ex); +}
165-180: Catch and log exceptions thrown by theTryAddToDBAsync,TrySendToCollectorAsync, andTryDeleteRecordsFromDBAsyncmethods.The
ProcessAsyncmethod does not handle exceptions thrown by theTryAddToDBAsync,TrySendToCollectorAsync, andTryDeleteRecordsFromDBAsyncmethods. It should catch and log exceptions to avoid terminating the method.Apply this diff to fix the issue:
try { await TryAddToDBAsync(records, corrId); } +catch (Exception ex) +{ + _logger?.LogError("{corrId} error while adding records to db in {queueName}\n{ex}", corrId, QueueName, ex); +} + +try +{ ITelemetryCollector<TRecord>.AddResult? result = await TrySendToCollectorAsync(records, corrId); +} +catch (Exception ex) +{ + _logger?.LogError("{corrId} error while sending records to collector in {queueName}\n{ex}", corrId, QueueName, ex); +} + +try +{ if (result != null && result.Successful) { await TryDeleteRecordsFromDBAsync(records, corrId); } } +catch (Exception ex) +{ + _logger?.LogError("{corrId} error while deleting records from db in {queueName}\n{ex}", corrId, QueueName, ex); +}
| _collector = config.Collector; | ||
| _retryPollingSize = config.RetryPollingSize; | ||
| _retryPollingInterval = config.RetryPollingInterval; | ||
| _cancellationTokenSource = new(); |
There was a problem hiding this comment.
Dispose _cancellationTokenSource when it is no longer needed.
The _cancellationTokenSource field is not disposed when it is no longer needed. This could lead to resource leaks. Consider disposing it in the Dispose method.
Apply this diff to fix the issue:
public void Dispose()
{
if (_isDisposed)
{
return;
}
_logger?.LogDebug("disposing {queueName}", QueueName);
_isDisposed = true;
_cancellationTokenSource.Cancel();
+ _cancellationTokenSource.Dispose();
_queue.Dispose();
_consumerTask.Wait(QueueFlushWaitTime);
}Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Codacy Static Code Analysis
[failure] 55-55: Ginger/GingerCoreNET/Telemetry/TelemetryQueue.cs#L55
Dispose '_cancellationTokenSource' when it is no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- Ginger/GingerCoreNET/Telemetry/Protos/V1/feature.proto (1 hunks)
- Ginger/GingerCoreNET/Telemetry/Protos/V1/log.proto (1 hunks)
- Ginger/GingerCoreNET/Telemetry/TelemetryBaseRecord.cs (1 hunks)
- Ginger/GingerCoreNET/Telemetry/TelemetryCollector.cs (1 hunks)
- Ginger/GingerCoreNET/Telemetry/TelemetryQueueManager.cs (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- Ginger/GingerCoreNET/Telemetry/Protos/V1/log.proto
- Ginger/GingerCoreNET/Telemetry/TelemetryBaseRecord.cs
- Ginger/GingerCoreNET/Telemetry/TelemetryCollector.cs
- Ginger/GingerCoreNET/Telemetry/TelemetryQueueManager.cs
Additional comments not posted (3)
Ginger/GingerCoreNET/Telemetry/Protos/V1/feature.proto (3)
5-15: LGTM!The
FeatureRecordmessage is well-structured and captures relevant information for feature telemetry. The field names are descriptive, and the field types are appropriate.
22-24: LGTM!The
AddFeaturesResponsemessage is well-structured and provides confirmation of the number of feature records added.
26-28: LGTM!The
FeatureCollectorservice and itsCollectmethod provide a clean and straightforward interface for collecting feature telemetry data.
Ginger/Ginger/ApplicationModelsLib/APIModels/APIModelWizard/WSDLParser.cs
Outdated
Show resolved
Hide resolved
…tion/Ginger into Features/Telemetry
added using wherever possible removed unnecessary imports
…tion/Ginger into Features/Telemetry
…tion/Ginger into Features/Telemetry
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
TelemetryMetadataclass for improved management and serialization of telemetry data.Bug Fixes
Documentation
Chores