Added simple WebApi using Correlate #132

[hartmark]

This is just a first draft for creating a CorrelationId for a .net framework project.

The logic happens in CorrelationIdActionFilter in the apitest project.

I needed to do a small change in CorrelationManager because I need to be able to call StartActivity and get the created context, I cannot use the method that wraps it in an action method.

Is there anything I have missed?

Things left to do:

• Move the CorrelationIdActionFilter class into a new nuget so it can be easier consumed
• Verify the logging with scope works aswell
• Add some unit-tests
• Check how I can support Correlate for downstream calls if I call another API from within my .net fw API

[skwasjer]

I needed to do a small change in CorrelationManager because I need to be able to call StartActivity and get the created context, I cannot use the method that wraps it in an action method.

This is not necessary. Just create the activity and then start it. The context will be created/managed for you.

string correlationId = null;
if (actionContext.Request.Headers.Contains(_options.RequestHeader))
{
    correlationId = actionContext.Request.Headers.GetValues(_options.RequestHeader).FirstOrDefault();
}

_activity = activityFactory.CreateActivity();
_activity.Start(correlationId ?? _correlationIdFactory.Create());

I would also not store the activity in a class field, but store it in the HttpActionContext and pull it from there to stop the activity later on. The issue currently is that the filter is registered as singleton, and not 'per request' so the field would be overwritten.

See CorrelateFeature how it is done in ASP.NET. The impl. should actually be very similar to that, using different options and some conditional logic for which headers to honor and if the response header should be added at all.

That said, I also think an impl. based on DelegatingHandler as per your OP is probably better than a filter, because it executes earlier in the request pipeline.

Possibly, CorrelateFeature could be extracted to the Core-lib, with an adapter pattern so we can reuse the logic for both use cases. (scratch that, it requires breaking changes/major version release, which we should avoid if you want this quick)

[skwasjer]

As for this:

Check how I can support Correlate for downstream calls if I call another API from within my .net fw API

This should just work the same, just register a HttpClient and attach the already built-in handler:

// Register a typed client that will include the correlation id in outgoing request.
services
    .AddHttpClient<IMyService, MyService>()
    .CorrelateRequests("X-Correlation-ID");

[hartmark]

New take, didn't see the CorrelateFeature class so I made a new implementation that uses System.Web.HttpContext that .net classic uses instead of Microsoft.AspNetCore.Http.HttpContext

I tried to make it as close as possible in implementation so I had to do some extension methods because .net core introduced the Try* methods for dictionaries.

I also moved some classes from AspnetCore to DependencyInjection projects because they are usable from .net framework aswell.

Do you have any comments on this implementation?

[skwasjer]

New take, didn't see the CorrelateFeature class so I made a new implementation that uses System.Web.HttpContext that .net classic uses instead of Microsoft.AspNetCore.Http.HttpContext

That is a nice approach, good one.

Do you have any comments on this implementation?

The direction is great but ideally, should still avoid introducing breaking changes if at all possible. While code duplication is not ideal, I prefer this for now if it means the public API contract of existing code remains unchanged. We can also still share private/internal code by linking files across each project too as an alternative.

I would say work towards getting this in a separate library with some test coverage as the primary goal.
Next, I have to figure out how to get this to build on GI actions because this is currently built with Linux/Ubuntu and I think this won't work:
https://github.com/skwasjer/Correlate/actions/runs/15704393791/job/44272824584#step:7:57

[hartmark]

Cool, my aim here was first to get your "blessing" on the solution before I make it more nuget-able.

I'll try on the next few days come up with a solution

[hartmark]

I created a new project Correlate.AspNet (feel free to give me a better name :))

With some trial and error I was able to move everything needed to setup into Correlate.AspNet

Next steps will be:

1. revert my breaking changes
2. add tests

Regarding tests, I noticed that if I add a x-correlation-id request header it is lost in my extension-method madness, so tests will be good to smoke out defects :P

PS. I'm working at my work computer at the moment, but my private computer is a linux so I can see if I can get it running with mono

[hartmark]

I have copied the CorrelateFeatureTests to CorrelateFeatureNet48Tests and have been able to smoke out all bugs I found.

What do you think @skwasjer ?

[hartmark]

I'm going on midsummer vacation for a few days.

I got it to compile successfully on linux porting the projects to sdk-style format.
hartmark@7bfdd04

I don't have access to my windows machine at the moment so I cannot test.

See #134 for how I solved it and please try it out on windows as I will not be able to do it now for some days.

[skwasjer]

I'm going on midsummer vacation for a few days.

Have fun!

I got it to compile successfully on linux porting the projects to sdk-style format. hartmark@7bfdd04

I don't have access to my windows machine at the moment so I cannot test.

See #134 for how I solved it and please try it out on windows as I will not be able to do it now for some days.

Seems to work 👍

I reviewed it with some suggestions.

[hartmark]

I have pulled in the csproj fix from the other PR and fixed some things you noted in the review.

[skwasjer]

I would still like to see some integration tests similar to what we have for the AspNetCore impl. Could maybe use Microsoft.Owin.Testing to host a test server?

The separate apitest project you have is fine for manual testing (and ideally should be moved to the ~/examples folder), but it does not really help with automated tests in GH actions.

[hartmark]

I think I have fixed all outstanding issues, please check if everything looks alright.

I'll try look into on how to do integration testing using Microsoft.Owin.Testing

[skwasjer]

I think I have fixed all outstanding issues, please check if everything looks alright.

LGTM

I'll try look into on how to do integration testing using Microsoft.Owin.Testing

👍

[hartmark]

I have added some integration tests now, it was a interesting task as I haven't used OWIN before. :)

[hartmark]

I have fixed the sonar cube violations and code coverage. Take a new look if everything looks alright.

https://sonarcloud.io/project/issues?pullRequest=133&open=AZejMj643MGkLmSCrrxF&id=skwasjer_Correlate
This violation is expected due to we only want to initialise once and it may run multiple times in the asp.net pipeline

I also got some duplication issues, but I'm not sure how to interpret the report.

[hartmark]

https://sonarcloud.io/project/issues?pullRequest=133&open=AZejMj643MGkLmSCrrxF&id=skwasjer_Correlate This violation is expected due to we only want to initialise once and it may run multiple times in the asp.net pipeline

I managed to fix this by using a static method instead

[skwasjer]

I have fixed the sonar cube violations and code coverage. Take a new look if everything looks alright.

https://sonarcloud.io/project/issues?pullRequest=133&open=AZejMj643MGkLmSCrrxF&id=skwasjer_Correlate This violation is expected due to we only want to initialise once and it may run multiple times in the asp.net pipeline

I also got some duplication issues, but I'm not sure how to interpret the report.

I have unfortunately not had much time this week to look over the PR, hopefully this weekend/early next week. Don't worry about the duplication, this is to be expected at this point, because of the similarities with the .NET Core impl.

I will rerun the pipeline at least to get an updated report. Thanks for your effort and patience 👍

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
98.8% Coverage on New Code
1.8% Duplication on New Code

See analysis details on SonarQube Cloud

[hartmark]

Did you get any time to look any more on this PR?