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

Header design #274

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Header design #274

wants to merge 1 commit into from

Conversation

hooksie1
Copy link

Referring to this PR nats-io/nats.go#1584 this includes an add() method to add a header key and value. This would facilitate extra context since the service doesn't have direct access to the request.

Also documents existing header methods (at least for the Go micro package).

Referring to this PR nats-io/nats.go#1584
this includes an add() method to add a header key and value. This
would facilitate extra context since the service doesn't have
direct access to the request.

Services should allow a header value to be added through the following method:

- add(key, value) - adds the value to the associated key or adds the key/value if the key doesn't exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should look at what the language specific otel text map propagator will need at least, in go that is

type TextMapCarrier interface {
	// Get returns the value associated with the passed key.
	Get(key string) string

	// Set stores the key-value pair.
	Set(key string, value string)

	// Keys lists the keys stored in this carrier.
	Keys() []string
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

The headers are there but they aren't able to be set. They return the Headers interface which just allows for getting values.

Copy link
Author

@hooksie1 hooksie1 Mar 24, 2024

Choose a reason for hiding this comment

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

Wrapping the nats.Header like Header(r.msg.Header) only allows the Get and Values methods

Screenshot 2024-03-23 at 20 39 44

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you want to set values on the request headers?

On the reply you can do all you want https://github.com/nats-io/natscli/blob/81e49cdfec59815a1a9b9d3cfd83b85e3db54406/cli/service_command.go#L74

Copy link
Author

Choose a reason for hiding this comment

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

That's why this was only allowing add and not set. I didn't want headers overwritten, just appended to.

The answer might be, don't forward the request and build a new one. That's more work but I guess is ok, however we will need a way to retrieve all headers. Currently there's only Get and Values. If a service wants to forward all headers (or copy headers from a response of another service) it would currently have to know all possible headers that can exist which feels like way too much overhead vs something like Headers.Copy().

Copy link
Author

@hooksie1 hooksie1 Mar 24, 2024

Choose a reason for hiding this comment

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

After typing that out, I think being able to copy all headers is the better approach. You can't just forward a request since the subject is part of the request. You'd have to make a new request anyway. I still think adding would be valuable for middleware but that might just be me.

I do however think that having a way to retrieve all headers is needed. That way when creating a new request you can just copy all headers into the new request and add your own. Also if service B responds to service A with a status code and also some other information, service A has to know and loop through every possible key. Having something like a Copy() method would allow you to just grab all headers from service B's response and tack them on to service A's response.

Copy link
Author

Choose a reason for hiding this comment

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

Or I'm dumb. Not at my computer but looking at the source. Does Headers() return the whole Message Headers or just return the interface that lets you call Get() and Values()?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s an interface but really it’s nats.Header so you can cast to that for full access.

BUT we of course don’t guarantee that you won’t be given a copy - now it’s not a copy in Go at least

Copy link
Author

Choose a reason for hiding this comment

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

Ok. Yeah I think it would be nice to have a copy method since casting is runtime and a copy would be safer.

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 this pull request may close these issues.

None yet

2 participants