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.

Sign up for GitHub

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

Jump to bottom

Header design #274

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

Header design #274

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions adr/ADR-32.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
| 2 | 2023-09-12 | Configurable queue group |
| 3 | 2023-10-07 | Add version regex info |
| 4 | 2023-11-10 | Explicit naming |
| 5 | 2024-03-16 | Header methods |

## Context and Problem Statement

Expand Down Expand Up @@ -352,3 +353,15 @@ will dispatch requests as fast as the handler returns.
For consistency of documentation and understanding by users, clients that implement the
service API and tooling that interacts with services should use the term "service" or
"services".


### Headers

Services should allow headers to be retrieved through two methods:

- get(key) - returns the first value associated with the key
- values(key) - returns all values associated with the key

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.

@hooksie1 Been chatting with @aricart and he pointed out that we already do headers, see:

https://github.com/nats-io/nats.go/blob/33316cdf884166bf6231192fa88304bb6533367c/micro/request.go#L57

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 •
edited
Loading

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 •
edited
Loading

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.