Azure-sdk-for-net: [FEATURE REQ] TryGetConfigurationSetting in Azure.Data.AppConfiguration.ConfigurationClient

[DavidStrickland0]

Library or service name.
Azure.Data.AppConfiguration

Is your feature request related to a problem? Please describe.
Currently when you request a Setting from Azure Configuration service that doesn't exist the Api returns a 404.
at https://github.com/Azure/azure-sdk-for-net/blob/94fdb6ba5719daa4d8d63b226c61064b2f52c085/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs#L568-L573
a 404 results in an exception being thrown.

Would be nice to have a
Response TryGetConfigurationSetting(string key, string label, DateTimeOffset acceptDateTime, MatchConditions requestOptions, out ConfigurationSetting setting, CancellationToken cancellationToken = default)
So we could try to get a setting without having to try catch every time.

Alternately just have a 404 return null instead of throwing

[AlexGhiondea]

@DavidStrickland0 thanks for filing this issue. We have discussed this in the past at length. /cc @annelo-msft.

We have decided against returning null on a 404 as that has the potential of ending up with a NullReferenceException somewhere in the code that is further away from where the call to the service happens, which would make this more difficult to diagnose.

We also considered adding a Try* method to retrieve a configuration setting. We didn't get to a good design here as the Try method needs to be async and async methods cannot have out parameters.

Could you share more about your scenario where you are trying to retrieve a setting that doesn't exist? We would like to learn more about how this API would be used.

[DavidStrickland0]

I have an Api that is exposed by multiple clients for use by their customers. We have our default settings in appsettings. If a client wants to change any of those settings we setup Azure configuration and we add whatever they want changed up into Azure Con.

Then when one of their customers makes a call to our Api via the Clients website the Api goes to AzureCon and gets any unique settings that client has and then proceeds to respond utilizing those new settings.

We want to keep our happy path as fast as possible so we don't want every call to check Azure(not to mention the cost deterrent). We only want AzureCon calls used for clients where they have chosen to accept the latency(and cost) in exchange for the customization. We also don't want to have to maintain a copy of every setting in our appsettings up in azure for every client that has at least one custom setting. We'd prefer any defaults the client doesnt want to change to just use the value from appsettings.
This basically leaves every call to a setting wrapped in something that looks like

        If(azureConConfigedForClient)
        {
            Try{ get company setting}
            Catch{ get default}
        }
        else
        {
             getdefault
        }

Needless to say using Try catch for flow control works, but is sub optimal.

Given the constraints you outline above it is sounds like a call to GetConfigurationSetting should return an instance of ConfigurationSetting with the Name and label matching what was requested but the value property = null. A string as null is pretty much a universal not set indicator in my book.
Or if an NRE on a string is still a concern Indicate it by type. Make ConfigurationSetting into IConfigurationSetting and make it an instance of NotSetConfigurationSetting when its missing or an instance of ConfigurationSetting when its set. Flow control by Type would be less odorous then flow control by non exceptional Exception.

Thanks for looking into it

[AlexGhiondea]

Thanks for describing your scenario and thanks for offering a potential fix.

We have generally veered away from interfaces as they don't have a versioning story. That is to say that there is no good way to add a new member on an existing interface without breaking backward compatibility.

We have also been consistently in the Track2 libraries (the new libraries that we are building) throwing an exception in the case of 4xx and 5xx status codes returned from the service.

At this point the best option seems to be what you already have in your code.

[DavidStrickland0]

We likely will have to rewrite this then eventually and make the Rest call directly but having this library was a nice time saver for our POC. Thanks for throwing it out here.

[AlexGhiondea]

@DavidStrickland0 glad to hear this was helpful for a POC. I would like to understand more about the need to go all the way to the REST layer in order to bypass the fact that we throw for a 404.

Are the concerns around performance, code style or something else that are pushing you away from the library? There are quite a few advantages that the library offers over raw requests to the REST endpoint (performance, distributed tracing, retries and many more).

[DavidStrickland0]

There seems to be a trend in a lot of code bases where exceptions are being accepted as the norm. They are being used to communicate information on the happy path. Its a trend I try to stay away from at all cost. I don't want our code base becoming apathetic toward exceptions. I fear that treating exceptions as commonplace will eventually lead to fragility so I just avoid it any time I can.
When I say Make the rest call direct I'd stay as high up in the stack as I can
CreateGetRequest unfortunately is private so I cant use it.
havent looked at _pipeline.SendRequestAsync if its worth it and I can use it I will

There are some libraries that we have to accept despite being "exception prone" because the gain out weighs the cost just not sure (at this point) this one qualifies. Of course we may well dig into it and find out it's actually harder then it appears and well worth it just haven't gotten that far yet.

[AlexGhiondea]

Thanks for your reply. I agree that exceptions should not be used for control flow. In this case though, I am not sure if that can be considered control flow or not -- that I think is fairly dependent on the application.

In the new libraries we have treated status codes such as 404 as an exceptional case and so we are throwing an exception in that case.

That being said, if you believe there is a solid reason to not throw for 404 I think we should discuss that and see if there is an API that we can come up with together that will work.

[DavidStrickland0]

If we take the Azure API as the whole then I can certainly see 404 as an Exception >80% of the time.
I was not considering the consistency of the API as a whole with the original proposal.

But looking over ConfigurationClient.cs I see a number of variations some calls treat only
200 as acceptable and everything else throws a exception
some allow 200 and 201
some allow 200 and 304
It appears that each call is addressing which status codes are acceptable and which status code produce an exception independently.

If the sdk is headed towards a centralized approach where there are certain hard and fast rules
"In the new libraries we have treated status codes such as 404 as an exceptional case and so we are throwing an exception in that case."

Then I would expect some sort of centralized result manager.

If there was such a centralized manager I would assume it would be injectable and thus overidable.

If the SDK is headed toward decentralization and every method controls status codes independently then the only thing we are considering is this one call in isolation and what other method calls are doing is irrelevant.

So I guess my question would be which way is the ball rolling?

[AlexGhiondea]

@DavidStrickland0 sorry for the late reply.

As we started the work on the new libraries, we spent a significant amount of time defining a set of general guidelines that apply to all new libraries (we call them 'Track 2 libraries') as well as a set of language specific guidelines.

In these Track 2 libraries we are striving to follow those guidelines as much as possible. There will always be exceptions but we are planning to be very intentional about those.

As you can see in the .NET guidelines on error handling the principle we are planning to follow is to ensure that every time we have a service with a non-success return code, that we throw a RequestFailedException exception.

Hopefully this gives you insight into what we are building and our reasons for it. I'm happy to chat about this some more if you have additional questions or feedback!

[DavidStrickland0]

Thanks Alex,
Typically when I want to share rules across multiple libraries I put those rules into a consumable that all the libraries use. This prevents having to rewrite the same code again and again. Once the standard is establish then a mechanism is encoded to allow for variance from that standard. Then each call can just use the default implementation and then if needed use the to code, filter or whatever to allow the variance.
If there is a way (whatever that way is) for individual methods to encode a variance presumably a consumer of the library would likewise be able to use that same mechanism to encode a variance that is appropriate in their application.

At the end of the day I just don't want to have to use Exception handling as flow control. I understand you have your own system constraints but a library that's prone to throw exceptions for common use cases isn't really something I'd be comfortable putting into production.

[AlexGhiondea]

@DavidStrickland0 the way we are going to enforce most of those is by autogenerating a large portion of the low-level, close to the wire code.

I understand your concern with the common use case for your application.

The library has been adopted by the AppConfiguration .NET provider and has gone through extensive testing to ensure that it can be used in a production environment.

[DavidStrickland0]

Actually sounds similar to our approach with our api. We gen'd the data tier with Roslyn. Each data tier call has an optional Func parameter. If the Func is set it handles any "error" condition. If the func isnt set or returns false it falls to the default handler that throws an exception.