search

elastic / elasticsearch-net

This strongly-typed, client library enables working with Elasticsearch. It is the official client maintained and supported by Elastic.
https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/index.html
Apache License 2.0
3.57k stars 1.15k forks source link

8.15.6 - General API Feedback while upgrading from NEST. #8339

Open niemyjski opened 1 week ago

niemyjski commented 1 week ago

Elastic.Clients.Elasticsearch version: 8.15.6

Elasticsearch version: 8.15.1

.NET runtime version: 8.x

Operating system version: Any

Description of the problem including expected versus actual behavior:

While upgrading I've noticed a few things that really made the conversion harder than it had to be and I feel like the feedback below would have made things easier. I get that the purpose of the rewrite was to make maintenance easier, but on the other end is consumers/customers that have large investments in existing api's / elastic.

  1. Defining new aggregations with a (dynamic agg name) using your object model is impossible now as you can't define agg names (only internal variant name and variant...) kinda makes the aggregation type useless for consumers.
  2. Kept the existing interfaces, they were not hurting anything especially around code gen.
  3. leverage interfaces more for very common fields like IProperty.Fields (tons of properties have fields, other common properties) having something like IHaveField or anything... This change with concrete types forces us to check tons of concrete types which makes maintaining it even harder as new types are added. No common interface for the tons of aggregation types seems like a miss?
  4. There is 0 point to have such an internalized api (worst case make the setters internal / private). This has made things much harder than it should be especially around aggregations / sorting. The property names chosen for some like VariantName kinda adds to the confusion.
  5. 1 way implicit conversions for aggregations requires lots of boxing, type casting, and harder to go back the other way, especially with internalized. Also this method might make your serializer implementation slightly easier to generate, but harder to work with. It also means the aggregations don't actually represent what is supported (e.g., meta, sub aggs).
  6. Leverage debugger displays / record types for better dx.
  7. Using library types like FluentDictionary for SearchRequest Aggregations. I get there is a constructor that takes a dictionary but using custom types like this does make using the descriptors a bit harder for some use cases.8. I've logged other issues below which seem like bigger problem areas to solve.Expected behavior

Easier to use api's and less internalized things leading to a better experience.

Reference: https://github.com/FoundatioFx/Foundatio.Parsers/pull/84

Also apologies if the labels are incorrect for or this issue:

Thank you.

flobernd commented 1 week ago

Hi @niemyjski, thank you for the detailed feedback.

Quite a few of these design decisions were made before I took over the maintainer role for the .NET clients. It probably makes sense to discuss some of these points during the call together with Martijn.

  1. If I understood correctly, you are mainly missing a way to implement custom aggregation variants (custom name, custom aggregation type) with the new client. Is that correct?
  2. The new client was pretty much built from the ground, so I imagine it was not really about "keeping" something but rather about "reimplementing" it. Martijn and Steve had very very tight deadlines to get a the first version going which required them to keep the scope very small for the beginning. This was more than a year before I joined, which means I can only speculate here as well.
  3. I can see how these kinds of interfaces might be useful for reflective code (especially in your parser project), but in the end, I still think the use-case is kinda nieche. I'm always open for improvements (having examples of a few more real world use-cases would surely help!), but at the same time, please remember that I'm currently the only person working on the .NET client (and related .NET projects which have high priority as well). This means that I have to prioritize very carefully.
  4. Setters are usually public where they should be public and internal when they are not meant to be used. One main reason is to avoid breaking changes when code generation internals are changing. This gives us a bit more freedom in the long term. The fields VariantName and Variant are using internal terminology and are marked as internal for this exact reason. However, I agree that there are cases where it would be desirable to have (controlled) access to some of the currently internal fields. In case of aggregations, this probably relates to 1. where I could definitely see me adding a public API that allows to create custom aggregations.
  5. Happy to do some brainstorming about simplifying the Aggregation type and to allow easier conversions etc. It's not that I don't see the benefits of the old structure. However, the new structuring does not only make serialization easier (and more performant!), but as well maps 1:1 to the actual ES REST API. This significantly reduces confusion for new users that just want to write their existing JSON queries using the .NET client.
  6. What are your major pain points here? Do you have concrete types in mind for which you would love to have better debugger representations? This, again, is something that can't easily be auto-generated which means I probably won't implement this for the broad range of types. It will require manual effort to do the initial implementation and as well increases the maintainance burden in the long term. I'm definitely open to implement this feature for some types where it really makes debugging a lot easier.
  7. What are you suggesting here? I could imagine adding an overload that directly accepts IDictionary<string, Action<AggregationDescriptor<T>> for example.
niemyjski commented 4 days ago
  1. Yes, this is easy todo with the descriptor but not easy to do if you want to say build up your own aggregations from a string and then set those onto a descriptor once everything is resolved.
  2. I get that, just don't get why they had to go, it makes migration that much harder for customers.
  3. See 2
  4. +1
  5. This sounds like a plan, let me try to work around #8334 and maybe this will solve some of my issues.
  6. If you use record types you get debugger displays and equality for free (and easier immutability). Just an observation I had looking through the code and will be way easier to debug agg based things once I am running.
  7. That would be my suggestion, use or have overloads for BCL types to make the api a bit easier to use. This was the first one I came across.