Skip to content

[Choregraphy] Upgrade EventGrid sdk from t1 to t2 #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hallihan merged 5 commits into from
Aug 17, 2021
Merged

[Choregraphy] Upgrade EventGrid sdk from t1 to t2 #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca7f917
Upgrade EventGrid to the new SDKs
zedy-wj Jun 10, 2021
4f2cb91
update codes
zedy-wj Jun 18, 2021
31e1259
use constant
zedy-wj Jun 22, 2021
00d9786
some update according to comments
zedy-wj Jun 25, 2021
a661909
update exception
v-yilinhu Aug 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 39 additions & 17 deletions ...reography/Fabrikam.Choreography.ChoreographyService/Controllers/ChoreographyController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
using Fabrikam.Choreography.ChoreographyService.Services;
using Fabrikam.Communicator.Service.Operations;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Azure.EventGrid;
using Microsoft.Azure.EventGrid.Models;
using Azure.Messaging.EventGrid;
Copy link

@jsquire jsquire Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truthfully, though we didn't change the behavior with the upgrade, we should really look at this controller in more depth. The logic is convoluted and it's demonstrating a really bad pattern of HTTP responses where any downstream failures are returned to the caller as an HTTP 400 (Bad Request) indicating "you did something wrong when sending your payload and you need to fix it for this to work."

Copy link

@jsquire jsquire Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coding conventions also often cut against best practices for C#. For example:

try
{
    ...
}
catch (Exception ex) when (ex is SomeExceptionType)
{
    ...
}

Should just be:

try
{
    ...
}
catch (SomeExceptionType ex)
{
    ...
}

Copy link
Contributor Author

@zedy-wj zedy-wj Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have known the existence of this problem, we will improve the logic after completing all migrations. Our primary task is upgrading sdk from t1 to t2.

Copy link

@jongio jongio Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this request expands the scope, but let's get a better design here as well. Do you want to take a stab at it with Jesse's recommendation above or do you need further guidance?

Copy link
Contributor Author

@zedy-wj zedy-wj Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will take a stab at it, but it maybe takes a few time.

Copy link

@jongio jongio Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you resolve this?

Copy link
Contributor

@v-yilinhu v-yilinhu Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jongio,@jsquire We tried to make some changes in exception handing. Do you think this is ok or we need to subdivide BadRequest exception further?

using Azure.Messaging.EventGrid.SystemEvents;
using Microsoft.Extensions.Logging;


Expand All @@ -28,11 +28,12 @@ public class ChoreographyController : ControllerBase
private readonly IDeliveryServiceCaller deliveryServiceCaller;
private readonly IEventRepository eventRepository;

public ChoreographyController(IPackageServiceCaller packageServiceCaller,
IDroneSchedulerServiceCaller droneSchedulerServiceCaller,
IDeliveryServiceCaller deliveryServiceCaller,
IEventRepository eventRepository,
ILogger<ChoreographyController> logger)
public ChoreographyController(
IPackageServiceCaller packageServiceCaller,
IDroneSchedulerServiceCaller droneSchedulerServiceCaller,
IDeliveryServiceCaller deliveryServiceCaller,
IEventRepository eventRepository,
ILogger<ChoreographyController> logger)
{
this.packageServiceCaller = packageServiceCaller;
this.droneSchedulerServiceCaller = droneSchedulerServiceCaller;
Expand All @@ -52,17 +53,25 @@ public async Task<IActionResult> Operation([FromBody] EventGridEvent[] events)

if (events == null)
{
logger.LogError("event is Null");
logger.LogError("event is Null");
return BadRequest("No Event for Choreography");
}

if (events[0].EventType is EventTypes.EventGridSubscriptionValidationEvent)
if (events[0].EventType is SystemEventNames.EventGridSubscriptionValidation)
{
try
{
var data = Operations.ConvertDataEventToType<SubscriptionValidationEventData>(events[0].Data);
var response = new SubscriptionValidationResponse(data.ValidationCode);
return Ok(response);
events[0].TryGetSystemEventData(out object systemEvent);
switch (systemEvent)
{
case SubscriptionValidationEventData subscriptionValidation:
return new OkObjectResult(new SubscriptionValidationResponse()
{
ValidationResponse = subscriptionValidation.ValidationCode
});
default:
break;
}
}
catch (NullReferenceException ex)
{
Expand Down Expand Up @@ -114,13 +123,22 @@ public async Task<IActionResult> Operation([FromBody] EventGridEvent[] events)
await eventRepository.SendEventAsync(listEvents);
return Ok("Created Package Completed");
}
catch (Exception ex) when (ex is BackendServiceCallFailedException ||
ex is EventException || ex is Exception)
catch (EventException ex)
{
logger.LogError(ex.Message, ex);
return BadRequest(ex);

}
catch (BackendServiceCallFailedException ex)
{
logger.LogError(ex.Message, ex);
return StatusCode(500);
}
catch (Exception ex)
{
logger.LogError(ex.Message, ex);
return BadRequest(ex);
}

}
case Operations.ChoreographyOperation.CreatePackage:
Expand All @@ -140,12 +158,16 @@ public async Task<IActionResult> Operation([FromBody] EventGridEvent[] events)
return Ok("Drone Completed");

}
catch (Exception ex) when (ex is BackendServiceCallFailedException ||
ex is EventException)
catch (EventException ex)
{
logger.LogError(ex.Message, ex);
return BadRequest(ex);
}
catch (BackendServiceCallFailedException ex)
{
logger.LogError(ex.Message, ex);
return StatusCode(500);
}
}
case Operations.ChoreographyOperation.GetDrone:
{
Expand All @@ -154,7 +176,7 @@ public async Task<IActionResult> Operation([FromBody] EventGridEvent[] events)
var deliverySchedule = await deliveryServiceCaller.ScheduleDeliveryAsync(delivery, e.Subject);
return Ok("Delivery Completed");
}
catch (Exception ex) when (ex is BackendServiceCallFailedException)
catch (BackendServiceCallFailedException ex)
{
logger.LogError(ex.Message, ex);
return BadRequest(ex);
Expand Down
3 changes: 2 additions & 1 deletion ...abrikam.Choreography.ChoreographyService/Fabrikam.Choreography.ChoreographyService.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Azure.Messaging.EventGrid" Version="4.4.0" />
<PackageReference Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.7.1" />
<PackageReference Include="Microsoft.ApplicationInsights.Kubernetes" Version="1.1.0" />
<PackageReference Include="Microsoft.AspNetCore.App" />
<PackageReference Include="Microsoft.AspNetCore.Razor.Design" Version="2.2.0" PrivateAssets="All" />
<PackageReference Include="Microsoft.Azure.EventGrid" Version="3.2.0" />
<PackageReference Include="Microsoft.Extensions.Configuration.AzureKeyVault" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.Logging.ApplicationInsights" Version="2.10.0" />
<PackageReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Design" Version="2.2.0" />
<PackageReference Include="NUnit" Version="3.13.2" />
<PackageReference Include="Serilog.AspNetCore" Version="2.1.1" />
<PackageReference Include="Serilog.Extensions.Logging.File" Version="1.1.0" />
<PackageReference Include="Serilog.Settings.Configuration" Version="3.1.0" />
Expand Down
30 changes: 13 additions & 17 deletions ...hy/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,48 +3,44 @@
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.Azure.EventGrid;
using Microsoft.Azure.EventGrid.Models;
using Azure;
using Azure.Messaging.EventGrid;
using Microsoft.Extensions.Logging;

namespace Fabrikam.Choreography.ChoreographyService.Services
{
public class EventRepository : IEventRepository
{
private readonly TopicCredentials topicCredentials;
private readonly EventGridClient eventGridClient;
private readonly string eventGridHost;
private readonly string[] Topics;
private readonly AzureKeyCredential topicCredentials;
private readonly EventGridPublisherClient eventGridClient;
private readonly string[] topics;
private readonly Random random;

public EventRepository(string eventGridHost,string eventKey, string Topics)
public EventRepository(string eventGridHost,string eventKey, string topics)
{
topicCredentials = new TopicCredentials(eventKey);
eventGridClient = new EventGridClient(topicCredentials);
this.eventGridHost = eventGridHost;
this.Topics = Topics.Split(",");
topicCredentials = new AzureKeyCredential(eventKey);
eventGridClient = new EventGridPublisherClient(new Uri(eventGridHost), topicCredentials);
this.topics = topics.Split(",");
random = new Random();
}

public string GetTopic()
{
return Topics[random.Next(0, Topics.Length)];
return topics[random.Next(0, topics.Length)];
}

public async Task SendEventAsync(List<EventGridEvent> listEvents)
{

try
{
var response = await eventGridClient.PublishEventsWithHttpMessagesAsync(eventGridHost, listEvents);
response.Response.EnsureSuccessStatusCode();
{
await eventGridClient.SendEventsAsync(listEvents);
}
catch (Exception ex) when (ex is ArgumentNullException ||
ex is InvalidOperationException ||
ex is HttpRequestException)
{
throw new EventException("Exception sending event to eventGrid", ex);

throw new RequestFailedException("Exception sending event to eventGrid", ex);
}


Expand Down
2 changes: 1 addition & 1 deletion ...y/src/choreography/Fabrikam.Choreography.ChoreographyService/Services/IEventRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Microsoft.Azure.EventGrid.Models;
using Azure.Messaging.EventGrid;
using System;
using System.Collections.Generic;
using System.Linq;
Expand Down
Loading