Feature/gh00839 use two web job instances to create messages to azure service bus based on content in busevent table #159
Conversation
…ded instance name in logging.
…s not handled by other instances.
|
Add link to issue in description? |
azure-pipeline.yml
Outdated
| name: equinor/procosys-infra | ||
| endpoint: 'equinor' | ||
| ref: master | ||
| ref: GH00839-Use-two-web-job-instances-to-create-messages-to-Azure-service-bus-based-on-content-in-busevent-table |
There was a problem hiding this comment.
This needs to be changed before merge I think?
There was a problem hiding this comment.
Yes. It will.
| await using var command = _context.Database.GetDbConnection().CreateCommand(); | ||
| command.CommandText = GetAllPlantsQuery(); | ||
|
|
||
| await using var result = await command.ExecuteReaderAsync(); |
There was a problem hiding this comment.
Would the code be simpler by using Dapper?
See EventRepository for examples like this:
var events = connection.Query(query.queryString, query.parameters).ToList()
There was a problem hiding this comment.
yes please, I think we have this exact code in Fam-feeder-function for reference
edit: seems we use ef core there, but dapper would be very suitable for this
There was a problem hiding this comment.
Why do we not have a DbSet configured for the Plant entity in the DbContext?
Would it not be way easier to just plants.Where(s => s.isVoided == "N").ToListAsync() as opposed to the current implementation?
There was a problem hiding this comment.
DbSet is configured for the Plant entity in the DbContext.
| if (events.Any()) | ||
| { | ||
| _logger.LogInformation("BusSenderService found {Count} messages to process after {Sw} ms", events.Count, | ||
| _logger.LogInformation("[{InstanceName}] BusSenderService found {Count} messages to process after {Sw} ms", _busEventRepository.GetInstanceName(), events.Count, |
There was a problem hiding this comment.
Curious, is there a simple way to add InstanceName to all traces under the hood, by some kind of setup of Application Insight?
There was a problem hiding this comment.
Looking into if that is possible. Eg adding it as telemetry.Context.GlobalProperties["InstanceName"] in initializer.
| } | ||
|
|
||
| private void TrackMessage(BusEvent busEvent, string busMessageMessageId, string busMessageBody) | ||
| private void TrackMessage(BusEvent busEvent, string busMessageMessageId, string busMessageBody, string? instanceName = null) |
There was a problem hiding this comment.
instanceName parameter is not used in method?
| await host.RunAsync(); | ||
| } | ||
|
|
||
| private static void LogConfiguration(IConfiguration configuration, ILogger logger) |
There was a problem hiding this comment.
Convenience method for listing available properties.
There was a problem hiding this comment.
But it's not in use, is there a reason to keep it?
There was a problem hiding this comment.
Removing it.
| using System.Text.Json.Serialization; | ||
|
|
||
| namespace Equinor.ProCoSys.BusSenderWorker.Core; | ||
| public class DateTimeConverter : JsonConverter<DateTime?> |
There was a problem hiding this comment.
Ser ikke helt hvorfor vi trenger denne klassen.
antar det er for å støtte datofromat slik det er satt opp i plantslease.json?
ie: "LastProcessed": "2025-02-17T14:01:40"
Hadde kanskje forventet at vi i stedet oppdaterte plantslease.json til å bruke ett standardformat som gjorde at vi ikke trengte en custom converter.
There was a problem hiding this comment.
Døme på tidsstempling standardformat: 2025-03-04T11:46:13.0961214Z viss me vil bruka det og unngå custom converter.
| _cache = cache; | ||
| } | ||
|
|
||
| public virtual List<string>? GetAllPlants() |
There was a problem hiding this comment.
Code smell for mocking purposes. Any good alternatives which are not too complex?
| return allPlants; | ||
| } | ||
|
|
||
| public List<string> GetPlantsHandledByInstance(List<PlantLease> plantLeases) |
There was a problem hiding this comment.
Name does not match anymore
There was a problem hiding this comment.
Could change method name to GetPlantsForCurrent since we are here looking at plant lease currently held (by the instance).
| var plantsHandledByCurrentInstance = new List<string>(); | ||
| var allPlants = GetAllPlants(); | ||
|
|
||
| if (allPlants != null && allPlants.Any()) |
There was a problem hiding this comment.
seems unlikely that allplants should be empty? And if it is, do we just want to carry on?
There was a problem hiding this comment.
Correct; we should not just carry on. Edge case, but this would indicate a potential issue with the data retrieval process, and throwing an exception with a relevant message helps in diagnosing the problem.
|
|
||
| if (allPlants != null && allPlants.Any()) | ||
| { | ||
| var plant = plantLeases.First(x => x.IsCurrent).Plant; |
There was a problem hiding this comment.
First() again. Double check if this is intended logic
| if (plant.Equals(PcsServiceBusInstanceConstants.RemainingPlants)) | ||
| { | ||
| var definedPlants = plantLeases.Where(x => !x.IsCurrent).Select(x => x.Plant).ToList(); | ||
| // We are also handling cases where RemainingPlants constant is used in combination with actual plants. E.g. PCS$TROLL_A, PCS$OSEBERG_C, REMAININGPLANTS. |
There was a problem hiding this comment.
// We are also handling cases where RemainingPlants constant is used in combination with actual plants. E.g. PCS$TROLL_A, PCS$OSEBERG_C, REMAININGPLANTS.
Is this still relevant?
There was a problem hiding this comment.
Maybe, maybe not. We will see after running it in test and production for a while. Hence not removing support for it.
…option to use single instance functionality.
…ttempt to reduce the number of calls to the latter method.
…eout defined for memory cache. Adjustments of cancellation token logic.
…ant this to complete in any case.
4 participants
https://github.com/equinor/cc-toolbox/issues/839