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

Allow re-using default connection config (custom properties) #152

Closed
slagiewka opened this issue Jan 17, 2023 · 4 comments · Fixed by #157
Closed

Allow re-using default connection config (custom properties) #152

slagiewka opened this issue Jan 17, 2023 · 4 comments · Fixed by #157
Assignees
Milestone

Comments

@slagiewka
Copy link
Contributor

When using SetClientConnectionName (to identify connections in RabbitMQ) it is impossible to use the built-in client properties of this library.

As per examples:

	config := amqp.Config{Properties: amqp.NewConnectionProperties()}
	config.Properties.SetClientConnectionName("sample-consumer")
	c.conn, err = amqp.DialConfig(amqpURI, config)

This results in default properties being ignored in openTune.

	if len(config.Properties) == 0 {
		config.Properties = Table{
			"product":  defaultProduct,
			"version":  buildVersion,
			"platform": platform,
		}
	}

In the end we get either {"connection_name": "sample-consumer"} or {"product": "AMQP 0.9.1 Client", "version": "1.5.0", "platform": "golang"}. Never both. Unless copied to client code with the risk of being outdated.

Of course, clients providing their own properties might not want to have these present. Because of that it would not make sense to "provide defaults if not present" in the properties table.

However some kind of union between default properties and client provided metadata would be beneficial.

Possible solutions:

  1. Expose amqp.DefaultConfig() that has Heartbeat, Locale and Properties set.
    With this, amqp.Dial(url) would be equal to amqp.DialConfig(url, amqp.DefaultConfig()) and setting other properties would go as follows:
config := amqp.DefaultConfig()
config.Properties.SetClientConnectionName("sample-consumer")
c.conn, err := amqp.DialConfig(amqpURI, config)
  1. Expose amqp.DefaultConfigProperties()
	config := amqp.Config{Properties: amqp.DefaultConfigProperties()}
	config.Properties.SetClientConnectionName("sample-consumer")
	c.conn, err = amqp.DialConfig(amqpURI, config)
  1. Both of the above if necessary
  2. Use other ways of using config, e.g. functional option pattern

Names TBD.

I'm up to create a Pull Request with changes, but let's discuss what are your expectations first.

@lukebakken lukebakken self-assigned this Jan 17, 2023
@lukebakken
Copy link
Contributor

I'm up to create a Pull Request with changes

Great, thank you. I suggest merging properties with the defaults, with the user-provided keys overwriting the default keys' values. If a user-provided value is nil, then remove the key from the Table. This would allow users to remove "product", for instance.

@lukebakken lukebakken added this to the 1.6.0 milestone Jan 17, 2023
@Zerpet
Copy link
Contributor

Zerpet commented Jan 17, 2023

Thank you for reporting this, good catch! I'm happy to a solution in the lines of point 2. The function NewConnectionProperties is a constructor of Table, so it makes sense to set default values there. I don't think we need a new function to get Table with defaults.

If somebody doesn't want to have those defaults, they can always t := make(Table). At the end of the day, Table is just an alias to map[string]any.

@slagiewka
Copy link
Contributor Author

Both of the above mean that for clients that have been using properties other than default, they will now how to verbosely opt-out of defaults.

Having properties (other than special cases defined by RMQ, like connection_name) is not harmful and setting them one way or the other is most likely not mission critical to any client. However, changing how that works now can be considered a breaking change.

Consider NewConnectionProperties. These have since their release been promising empty value Table. Users would expect that to be empty. It's obviously problematic to have documentation of this public function specify implementation details.

I will provide a change to NewConnectionProperties, but will leave it to you if you want to proceed without releasing new major.

@Zerpet
Copy link
Contributor

Zerpet commented Jan 18, 2023

Both of the above mean that for clients that have been using properties other than default, they will now how to verbosely opt-out of defaults.

This is true. I would counter argue that NewConnectionProperties was added in 1.5.0, the latest release as of today, so I don't expect this function to be widely used. I'd expect the vast majority of code bases out there to initialise their connection properties wither either make or the zero-value Table{}.

IMO, constructors should set sane defaults, in addition to initialising the type. This missing bit in the constructor is arguably a bug.

Users who want to opt-out from the defaults can initialise the properties like:

p := amqp.Table{"connection_name": "my-connection"}
config := amqp.Config{Properties: p}

This has been the pattern up to 1.5.0, where NewConnectionProperties() was introduced.

It's obviously problematic to have documentation of this public function specify implementation details.

Indeed, we should fix the documentation alongside with the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants