spring-cloud / spring-cloud-consul

Spring Cloud Consul
http://cloud.spring.io/spring-cloud-consul/
Apache License 2.0
813 stars 541 forks source link

Removal of "tags as metadata" breaks filtering service lookup by dynamic tag value #682

Closed onobc closed 3 years ago

onobc commented 3 years ago

In this commit the "tags as metadata" ability was removed.

This was used in 2 areas (that I know of): 1) During service registration 2) During service lookup

This is a breaking change for us due to #2.

Our services are not registered via Spring Cloud Consul Discovery but we do use it to lookup services in consul (#2 above). We have 1000s of service instances that do not use metadata and instead use a tag to identify the "name" of the service instance. We find all service instances and then we spin through and filter those w/ the desired tag value (in the consul metadata map). This quit working as the metadata map is now empty.

Here are some options that come to mind:

1) Update all of our services to properly use metadata when they are registered

2) Set the spring.cloud.consul.discovery.default-query-tag: myServiceTag

3) Re-instate the ability to use "tags as metadata" for use case #2

4) Re-instate the "tags as metadata" feature but default it to "off"

I am willing to submit a merge request for whatever option the team lands on.

onobc commented 3 years ago

@spencergibb can you take a scan of this ticket? This will be a blocker for us to upgrade to 2020.0.0.

spencergibb commented 3 years ago

Indeed, 2020.0 was a chance for us to make breaking changes. Tags as metadata was a hack/workaround.

We find all service instances and then we spin through and filter those w/ the desired tag value

Since that is your current mode of operations, I've created a ConsulServiceInstance class that you may cast to that contains a getTags() method and a getHealthService() if there is any other data anyone might need that isn't in the interface.

onobc commented 3 years ago

Thanks @spencergibb - this will work. I do also like having the ability to get at the backing health service if any other "need" arises later.

However, is there any reason we would not want to support multiple default query tags? If we were able to set multiple query tags it would avoid having to pull back 1000 services and spin through them. Instead, it would pull back the small subset that matched the query tags.

I am guessing one reason would be that there are already enough moving parts w/ 2020.0 that anything extra can wait. If thats the case then I could always submit a merge request later once the dust settles w/ the suggested improvement.

Again, thank you for getting this commit in. It is much appreciated.

spencergibb commented 3 years ago

Yes, I'll look at your pr soon. I'm not opposed to multiple default query tags