Skip to content
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.

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

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

Open
fgheysels opened this issue Apr 1, 2022 · 12 comments

Comments

@fgheysels
Copy link
Member

fgheysels commented Apr 1, 2022

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

  • is it the responsability of Arcus to provide a method (extension method on HttpClient which easily allows developers to set the correlation-info ?
  • Is it the responsability of Arcus to provide functionality to easily set correlation-info in servicebus messages ?

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
Copy link
Member

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
Copy link
Member

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

@stijnmoreels
Copy link
Member

stijnmoreels commented Apr 15, 2022

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:

public static MessageCorrelationInfo GetCorrelationInfo(this Message message, string transactionIdPropertyName)

Added an issue for this: arcus-azure/arcus.messaging#269

@stijnmoreels
Copy link
Member

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:

var upstreamOperationParentId = $"|{correlationInfo?.OperationId}.{newDependencyId}";

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
Copy link
Member

stijnmoreels commented Jun 8, 2022

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
Copy link
Member Author

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
Copy link
Member

stijnmoreels commented Jun 8, 2022

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants