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

resolve the prometheus discovery issue of same service‘s multiple instances #3073

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

deanwong
Copy link

@deanwong deanwong commented Jan 18, 2025

Description
When setAutoPorts enabled, if starting two rpc service (e.g. user rpc service) , the second one will overwrite the etcd value with same key, the code in start.go as below

if autoSetPorts {
	listener, prometheusPort, err = getAutoPort()
	if err != nil {
		return err
	}

	etcdClient := client.(*etcd.SvcDiscoveryRegistryImpl).GetClient()
        // this step will overwrtite the same service with different value
	_, err = etcdClient.Put(ctx, prommetrics.BuildDiscoveryKey(rpcRegisterName), jsonutil.StructToJsonString(prommetrics.BuildDefaultTarget(registerIP, prometheusPort)))
	if err != nil {
		return errs.WrapMsg(err, "etcd put err")
	}
}

For example, I started user service, the etcd value like

openim/prometheus_discovery/user {"target":"10.102.22.175:40145","labels":{"namespace":"default"}}

if I started another user service in different node, the etcd value was covered

openim/prometheus_discovery/user {"target":"/168.64.9.42:34657","labels":{"namespace":"default"}}

Fixed
Add nodeId and port in etcd key, the value will be like this in same scenario

openim/prometheus_discovery/user/10.102.22.175:40145 {"target":"10.102.22.175:40145","labels":{"namespace":"default"}}
openim/prometheus_discovery/user/168.64.9.42:34657 {"target":"/168.64.9.42:34657","labels":{"namespace":"default"}}

Moreover, the prometheus discovery will return correct node instances,

[
    {
        "targets": [
            "10.102.22.175:40145",
            "168.64.9.42:34657"
        ],
        "labels": {
            "namespace": "default"
        }
    }
]

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 18, 2025
Copy link

github-actions bot commented Jan 18, 2025

🤖 All Contributors have signed the CLA.
The signed information is recorded here
Posted by the CLA Assistant Lite bot.

@deanwong
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

OpenIM-Robot added a commit to OpenIM-Robot/cla that referenced this pull request Jan 18, 2025
@deanwong
Copy link
Author

I found that the registration to etcd does not consider using a lease, which will be fixed later

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant