CSE internal feedback - Justin

[priyaananthasankar]

Serverless Microservices Feedback

Review of https://github.com/Azure-Samples/Serverless-Eventing-Platform-for-Microservices

Both from CI / CD setup process and from code of Microservices

CI / CD

Overall - very impressed with what things are at currently. This is one of the best articles and guides to setting up CI / CD for Microservices so far.

Having the yaml files is awesome. If we can get the VSTS team to fix the functionality that supposedly works https://docs.microsoft.com/en-us/vsts/pipelines/build/yaml?view=vsts#automatically-create-a-yaml-build-definition we should consider switching to that. It would avoid all the manual importing of things.

Would be nice if they supported the yaml for the Release page as well, but maybe we could provide the json instead?

We should consider using a hard coded suffix and allowing for custom prefixes. Most developers that we have worked with together tend to do that. So for instance naming could be jwaudioapi vs. crcaapi. It's just easier to remember the host names for building out the release tasks.

In the EventSubscription Tasks we should consider stating to use the "Events" resource group in the task editor. This got me several times.
9. Deploy Event Subscriptions ARM Template: Use the Azure Resource Group Deployment task, with the Action set to
Create or update resource group . Set the Template to the location Of the eventGridSubscriptions.json file, e.g.
. Set the template
parameters to the following: -eventGridTopicName {event-grid-topic-name} -microserviceResourceGroupName
ContentReactor-Images -microseNiceFunctionsWorkerApiAppName {worker-api-function-app-name}

Could we consider splitting the doc at https://github.com/Azure-Samples/Serverless-Eventing-Platform-for-Microservices/blob/master/setup.md into two parts? One that is Build and one that is Deploy. Then we could use the setup.md page to describe why we are splitting the two steps and can point to references on CI / CD for Microservices etc. It just seemed like a bit wall of text of steps to walk through. Some devs might even pass it up and say "too much work" etc.

Code Review

Using HttpRequestMessage vs. HttpRequest… Found this in several spots
// get request body
var requestdody = await new StreamReader(req.Body) .ReadToEndAsync();
ccmp1etecreateAudioRequest data;
try
data
•sonconvert. Deser iali zeobject< ;
catch OsonReaderException)
return new BadRequest0bjectResult(new { error
"Body should be provided in JSON format."

We should consider using the less JsonConvert approach?

[FunctionName(" RegistrationGenerator")]
public static async Task
var simulationltems = await req. Content. ReadAsAsync<IEnumerab1e>();
// Function input comes from the request content.
string instanceld = await
log. Info($ "Started orchestration With ID
"get",
req,
simulationltems) ;

Loved seeing services / repositories being used against interfaces!!!
private const string JsonContentType
" application/json " ;
public static IAudioService AudioSerwice = new BlobRepository(), new AudioTranscriptionService(), ne
public static ILIserAuthenticationSerwice UserAuthenticationService = neu

Wondering though if we should consider using DependencyInjection or if it would overcomplicate a simple example? We could use ServiceCollection and a ServiceProviderLocator pattern to build helpers around it etc. This way everything doesn't need to be public static (above) and can be singleton instead in the DI container.

(Also - on a side node, we should consider making those services private static, public static uses up a different heapsize because it's outside the user process space for that method call).

Using attributes instead of copying code everywhere
// get the user ID
if ( ! await User-AuthenticationSerwice.GetUserIdAsync (req,
return responseResuIt;
out var userld,
out var responseResuIt))
Would love to see this be a part of the function rather than explicitly copied in each method. (Would love to see it through Azure/azure-functions-host#33) but meh - never going to happen. Maybe a pattern like this instead? https://github.com/stuartleeks/AzureFunctionsEasyAuth/blob/master/src/FunctionWithAuth/AuthFunctions.cs

Inside ListAudio under Audio.Api we are using ContentResult instead of just returning an OkObjectResult. Just wondering why? Seems inconsistent with other calls.
return new ContentResuIt
Content = json,
ContentType
StatusCode =
= JsonContentType,
Statu sCode s. Statu s2øøOk

Inside Audio.Services AudioTranscriptionService.cs we should consider using the SDK for cognitive services.
protected internal HttpuebRequest CreateAudioTranscriptRequest(Stream audiogIobStream)
var request = ;
request . SendChunked = true;
request . Accept =
request.Method = "POST";
request . Protocolversion = HttpVersion.VersionII;
request .ContentType = codec—audio/ pcm; ;
request.
if (audioBlobstream.canseek)
aud posi tion

We should consider using the EventGridTrigger from the extensions sdk vs. using an HttpTrigger and handling the eventgrid through the client sdk
[Punct ionName ( " UpdateCategorySynonyms " ) ]
public static async UpdateCategorySynonyms(
[RttpTrigger(AuthorizationLeveI. Function, req,
TraceUriter log)
// authenticate to Event Grid if this is a validation event
var eventGridVaIidationOutput = EventGridSubscriberSerwice.HandIeSubscriptionVaIidationEvent(req) ;
if (eventGridVaIidationOutput null)
log. Info( "Responding to Event Grid subscription verification. " ) ;
return eventGridVaIidationOutput;
try
var ( _ , userld, categoryld) = EventGridSubscriberService. DeconstructEventGridmessage(req);
// process the category Synonyms
synonyms for category ID {categoryld}.
var updated = await CategoriesSerWice.UpdateCategorySynonymsAsync(categoryId,
if (lupdated)
userld) ;
not update category synonyms as no synonyms were available.
return new OkResuIt();

For the unit tests, we should be more consistent. Right now some places use Moq and other places create a mock class
[Fact]
public async Task
// arrange
Environment . "https://fake/");
Environment . SetEnvironmentVariabIe( "CognitiveSerwicesSearchApiKey"
"tempkey" ;
new ImageSearchSerwice(new Random(), new HttpCIient(new
var service =
// act
var result
= await service.

Code Coverage should be verified - some classes like the services have excellent coverage
https://github.com/Azure-Samples/Serverless-Eventing-Platform-for-Microservices/blob/master/categories/src/ContentReactor.Categories/ContentReactor.Categories.Services.Tests/CategoriesServiceTests.cs

Other classes are really just testing "happy path".

Instead of locking we should consider using concurrentdictionary as it comes for free (and does it better than we can)
Machine generated alternative text:
private readonly Dictionary<T, Hashset>
new Dictionary<T, Hashset>();
public int Count
connections . Count;
public void Add(T key, string connectionld)
lock ( connections)
Hashset clientconnections;
connections
if (! connections. TryGetVa1ue(key, out clientconnections))
clientconnections
new Hashset();
connections .Add(key, clientconnections);
lock (c:lientconnections)
clientconnections . Add (connectionld) ;

Minor one here, but we should consider returning model objects instead of IActionResult inside any class that's considered a "service or repository" mainly because if we move it to a different library, it will have a dependency on System.Web.

For example https://github.com/Azure-Samples/Serverless-Eventing-Platform-for-Microservices/blob/master/web/src/signalr-web/SignalRMiddleware/SignalRMiddleware/Services/NotificationService.cs

Machine generated alternative text:
public IActionResu1t SendEventVa1idationResponse(string code)
logger . Loglnformation("Received validation request from event grid .
EventVa1idationResponse response
new EventVa1idationResponse();
response.Va1idationResponse = code;
return new OkobjectResu1t(response) ;

• code);

Just out of curiosity what's the difference between (in here https://github.com/Azure-Samples/Serverless-Eventing-Platform-for-Microservices/blob/master/shared/src/ContentReactor.Shared/EventGridPublisherService.cs)
Machine generated alternative text:
// publish the events
var client
new EventGridC1ient(topicCredentia1s);
return client. publishEventsWithHttpMessagesAsync(topicEndpointHostname,
events) ;

And

Client.PublishEventsAsync()?

[irajbalakrish]

can we break this enhancement into simpler pieces to be able to fix it in smaller chunks. May be we can update Angular to the latest version and refresh the UI with material ( that's an overreach for now but just saying )