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

Discovery public API review and refactorings #1280

Merged
merged 25 commits into from
Apr 20, 2024
Merged

Conversation

bart-vmware
Copy link
Member

@bart-vmware bart-vmware commented Apr 18, 2024

Description

This PR is the result of the public API review for Steeltoe Service Discovery, along with various bug fixes, improvements, and refactorings. Changes are listed below.

General

  • Remove unused/redundant interfaces and base classes: they seemed great for extensibility, however their structures matched with one very specific implementation
  • Use IOptions pattern and respond to configuration changes
  • Applied Public API guidance from Public API Surface Area Review #897 (comment)
  • More async all-the-way, reduced sync-over-async usage
  • Types and members are annotated for nullable reference types
  • Improved test coverage, testing more execution flows instead of single classes
  • Improved logging output, which enables to track what's going on with relevant contextual information
  • Lots of corrections in triple-slash documentation comments, more types are documented now
  • Remove redundant [InternalsVisibleTo] entries, optimize project dependencies
  • Use dependency injection in most places, remove public static instances
  • Drop usage of IServiceInfo and related assembly scanning attributes
  • Remove redundant HostBuilder extensions, in favor of extensions on IServiceCollection
  • Throw NotSupportedException instead of NotImplementedException for things that were deliberately not implemented
  • Throw FormatException instead of InvalidOperationException when the conversion of primitive type fails
  • Bootstrap support for using multiple active discovery clients (see below)

Load balancing

  • Improve reliability of round-robin load balancer when used without distributed cache, by atomically switching to the next available service (fixes race condition)
  • NoOpDiscoveryClient (and related warnings) has been removed, as load balancers can query multiple discovery clients now
  • Moved consumption of service discovery (HttpClient support and load balancers) to Steeltoe.Discovery.HttpClients project
  • Simpler API surface: use IHttpClientBuilder.AddServiceDiscovery<TLoadBalancer>() for a named client, or use DiscoveryHttpClientHandler for non-factory scenarios

Service Discovery

  • Remove project Steeltoe.Discovery.Abstractions, move configuration-based discovery into new project Steeltoe.Discovery.Configuration
  • Merge IServiceInstanceProvider into IDiscoveryClient, moved to Steeltoe.Common.Abstractions
  • Remove AddServiceDiscovery and AddDiscoveryClient (registration) extension methods (which used assembly scanning), in favor of client-specific extension methods with explicit dependencies
  • Multiple discovery clients can be active at the same time (one per type), Eureka/Consul can be turned off via configuration
  • Remove IDiscoveryClientExtension, in favor of using the built-in D/I container
  • Support for global service discovery (for all HttpClients within the app), ie: services.ConfigureHttpClientDefaults(builder => builder.AddServiceDiscovery());

Consul

  • Use PeriodicTimer for background heartbeats (async and cancellable, no overlaps)
  • Implement IValidateOptions, so an OptionsValidationException is thrown on an invalid configuration
  • Remove setting consul:discovery:cacheTtl, which used to set global DistributedCacheEntryOptions for the entire app (dangerous and non-intuitive), you can still register this yourself, which will be picked up
  • Rename undocumented setting consul:discovery:UseNetUtils to consul:discovery:UseNetworkInterfaces
  • Improved detection of ASP.NET binding to hosts/IPs and ports, including the new HTTP_PORTS/HTTPS_PORTS environment variables from .NET 8
  • Simplify namespaces, ie Steeltoe.Discovery.Consul.Discovery to Steeltoe.Discovery.Consul

Eureka

  • Replace reflection-based fetching of management endpoint configuration (health, info) with light-up dependency
  • Rename ScopedEurekaHealthCheckHandler to EurekaHealthCheckHandler
  • Fix various race conditions and potentially sending a partially-updated (inconsistent) instance to Eureka
  • More efficient registration/renewal, only sending non-null properties
  • Optimized execution flow for periodic renewals and heartbeats, not sending more frequently than needed
  • Internally use a UTC-based DateTime for timestamps, instead of long (which could be .NET ticks or Java ticks).
  • Implement IValidateOptions, so an OptionsValidationException is thrown on an invalid configuration
  • Prefer secure port when both secure/non-secure are returned from Eureka
  • Support ASP.NET dynamic port assignment (as long as it doesn't change the local Instance ID)
  • Internally use TimeSpan for timeouts and intervals, instead of seconds/milliseconds
  • Fixes in determining HomePage/Status/HealthCheck URLs, support runtime-replacement of ${eureka.hostname}
  • Communicate with Eureka using named HttpClient from HttpClientFactory, so the handler pipeline can be tweaked as desired (ie custom headers)
  • Fixed: eureka:client:EurekaServer:RetryCount indicates the number of retries, instead of attempts
  • Fixed: unhandled exception when trying to mask URL during log when the URL is invalid
  • Perf: reuse JsonSerializerOptions instead of recreating it on every request
  • Remove setting eureka:client:shouldOnDemandUpdateStatusChange, updates are always sent immediately when instance changes (from configuration or user code)
  • Remove setting eureka:instance:defaultAddressResolutionOrder because it was never used nor documented
  • Remove setting eureka:client:cacheTtl, which used to set global DistributedCacheEntryOptions for the entire app (dangerous and non-intuitive), you can still register this yourself, which will be picked up
  • Rename undocumented setting eureka:instance:UseNetUtils to eureka:instance:UseNetworkInterfaces
  • Support comma-separated list of multiple names in setting eureka:instance:vipAddress and eureka:instance:secureVipAddress
  • Rename [Secure]VirtualHostName to [Secure]VipAddress, using multiple terms for the same thing is confusing
  • Fixed: Take OverriddenStatus into account when evaluating instance status
  • Config Server discovery-first now supports multiple discovery clients with light-up dependencies
  • Improved detection of ASP.NET binding to hosts/IPs and ports, including the new HTTP_PORTS/HTTPS_PORTS environment variables from .NET 8
  • Don't infer hostname from the URLS environment variable (ASP.NET ignores it, so will Steeltoe)
  • Both secure and non-secure ports are initially disabled in configuration. Steeltoe probes for URLS/HTTP_PORTS/HTTPS_PORTS and activates them, based on what the app binds to. In Cloud Foundry, this information is inferred from the cloud environment.

Fixes #901, fixes #1107, fixes #829, fixes #786, fixes #493, fixes #480, fixes #479, fixes #121, fixes #11.

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

@bart-vmware
Copy link
Member Author

bart-vmware commented Apr 18, 2024

/azp run Steeltoe.All

Copy link

No pipelines are associated with this pull request.

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bart-vmware bart-vmware added Component/Discovery Issues related to Steeltoe Service Discovery ReleaseLine/4.x Identified as a feature/fix for the 4.x release line labels Apr 18, 2024
@bart-vmware bart-vmware marked this pull request as ready for review April 18, 2024 15:11
@TimHess TimHess added this to the 4.0.0-m1 milestone Apr 18, 2024
Copy link
Member

@TimHess TimHess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks like a huge improvement, quite a bit easier to read and understand

bart-vmware and others added 3 commits April 19, 2024 22:33
@bart-vmware
Copy link
Member Author

bart-vmware commented Apr 19, 2024

/azp run Steeltoe.All

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

TimHess
TimHess previously approved these changes Apr 19, 2024
Copy link
Member

@TimHess TimHess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

sonarcloud bot commented Apr 19, 2024

@bart-vmware bart-vmware merged commit fe0a001 into main Apr 20, 2024
24 checks passed
@bart-vmware bart-vmware deleted the discovery-api-review branch April 20, 2024 06:24
@osmedd
Copy link

osmedd commented Apr 20, 2024

Thank you! This is a huge win I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment