arcus-azure / arcus-service-to-service-correlation-poc

POC to have end-to-end correlation stitching operations across services together in the Azure Application Insights Application Map.
MIT License
0 stars 3 forks source link

Is it the responsability of Arcus to enable adding correlation-id's to dependency calls ? #6

Open fgheysels opened 2 years ago

fgheysels commented 2 years ago

Is it the responsability of Arcus to provide functionality for developers to easy set 'dependencyIds' when calling dependencies ?
For instance:

I would say: no, this is not the responsability of Arcus. (At least, not in a first phase).

I think we should focusing on the 'accepting' end.
That is, we should be able to log requests, and correlate incoming requests with the dependency call that triggered that request. To do that, we would need methods that would allow developers to specify a delegate on how to retrieve the correlation-information from the incoming request (http request, servicebus message, .... )

That is, when configuring the Correlation Middelware in API / ASP.NET projects, developers must be able to specify how the correlation information must be retrieved Same is true when registering a message-pump that processes servicebus messages

stijnmoreels commented 2 years ago

Yep, I think that would be a good first step. I think it will be very valuable to also document this in a cross-repo way. Or some sort. That we can show people how to set this up. Even when in the first phase we don't provide built-in functionality that does the 'add' of correlation information in HTTP and/or Azure Service Bus messages.

stijnmoreels commented 2 years ago

This is more of a discussion, really, than an issue 😄 .

stijnmoreels commented 2 years ago

That is, when configuring the Correlation Middelware in API / ASP.NET projects, developers must be able to specify how the correlation information must be retrieved

This is already the case I think as the HTTP correlation middleware in Web API has several options to how the correlation should be extracted from the request, how the response should be structure, and many more... https://webapi.arcus-azure.net/features/correlation

We do not have a configuration value to specify the location of the operation parent ID in the Service Bus message. This should be able to configure on the message.GetCorrelationInfo extension as this is both used in the Worker/Azure Functions scenario and will result in a Serilog enricher of the message correlation. You can see that here: https://github.com/arcus-azure/arcus-service-to-service-correlation-poc/blob/12dba574c89a4aa9ea3e192d5887f5749b298f2e/src/Arcus.POC.Messaging.ServiceBus.Core/Extensions/MessageExtensions.cs#L50

Added an issue for this: https://github.com/arcus-azure/arcus.messaging/issues/269

stijnmoreels commented 2 years ago

Creating the 'next' dependency ID is probably a good thing to be included in Arcus, otherwise the service-to-service functionality is not at all user-friendly. Talking about this line for Service Bus dependencies: https://github.com/arcus-azure/arcus-service-to-service-correlation-poc/blob/dad99894d7080cf44e5f7e1e58ded7421b9a4826/src/Arcus.API.Market/Repositories/OrderRepository.cs#L46 We could provide an easier way to come up with this stuff, as it's not only obscure of new people, it's hard to get right.

stijnmoreels commented 2 years ago

Something like this or so?

var orderRequest = new EatBaconRequestMessage
{
    Amount = amount
};

using (var serviceBusDependencyMeasurement = DurationMeasurement.Start())
{
    bool isSuccessful = false;
    CorrelationInfo currentCorrelation = _correlationInfoAccessor.GetCorrelationInfo();
    CorrelationInfo newDependencyCorrelation = currentCorrelation.CreateForNextDependency();

    try
    {
        var serviceBusMessage = ServiceBusMessageBuilder.CreateForBody(orderRequest)
                                                    .WithCorrelationInfo(newDependencyCorrelation)
                                                    .Build();

        await _serviceBusOrderSender.SendMessageAsync(serviceBusMessage);

        isSuccessful = true;
    }
    finally
    {

        var serviceBusEndpoint = _serviceBusOrderSender.FullyQualifiedNamespace;
        _logger.LogServiceBusQueueDependency(_serviceBusOrderSender.EntityPath, isSuccessful, serviceBusDependencyMeasurement, dependencyId: newDependencyCorrelation.OperationParentId);
    }
}

We could also hide the entire measurement/dependency tracking behind an extension on the ServiceBusSender type, of course, (like we also hide the stuff in the HTTP variants).

var orderRequest = new EatBaconRequestMessage
{
    Amount = amount
};
var correlationInfo = _correlationInfoAccessor.GetCorrelation();
await _serviceBusOrderSender.SendMessageAsync(serviceBusMessage, correlationInfo);

We'll have to check, maybe if the new Azure SDK supports back extensions, we can even hide it in the creation of the ServiceBusSender.

fgheysels commented 2 years ago

I think it is mandatory to not put a burden on the developer to define / generate those Id's.

If possible, I'd like to make sure that the Arcus user doesn't even need to think about generating a dependencyId, even if it just means that a certain function needs to be called. Can't we include the generation of that Id in the LogDependency extension method ?

stijnmoreels commented 2 years ago

I think it is mandatory to not put a burden on the developer to define / generate those Id's.

Yes, isn't then the second example the thing that you want? Where the tracking is completely hidden?

Can't we include the generation of that Id in the LogDependency extension method ?

Well, no, bc the measurement of the call to the dependency should come before that, so then it's already too late.

fgheysels commented 2 years ago

I think it is mandatory to not put a burden on the developer to define / generate those Id's.

Yes, isn't then the second example the thing that you want? Where the tracking is completely hidden?

Can't we include the generation of that Id in the LogDependency extension method ?

Well, no, bc the measurement of the call to the dependency should come before that, so then it's already too late.

You can only call the LogDependency method when the dependency call is finished no ? Because you need to know whether the call was succesfull and you also need to know how long the dependency call took.

stijnmoreels commented 2 years ago

You can only call the LogDependency method when the dependency call is finished no ? Because you need to know whether the call was succesfull and you also need to know how long the dependency call took.

Yes, exactly, so that's why the dependency ID can't be generated by the LogDependency.

stijnmoreels commented 2 years ago

So, to come back: we can opt for a way to hide the dependency ID completely in a separated overload (2th code sample) or via simpler methods on something else (1th code sample).

fgheysels commented 2 years ago

You can only call the LogDependency method when the dependency call is finished no ? Because you need to know whether the call was succesfull and you also need to know how long the dependency call took.

Yes, exactly, so that's why the dependency ID can't be generated by the LogDependency.

Yes, true. I might have been confused as I was focusing on 'parent Ids' yesterday.

fgheysels commented 2 years ago

So, to come back: we can opt for a way to hide the dependency ID completely in a separated overload (2th code sample) or via simpler methods on something else (1th code sample).

I like the 1st sample best, where the correlation information is specified when creating the message.