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

Update Setter used for Injection #444

Open
carlosalberto opened this issue Feb 5, 2020 · 7 comments
Open

Update Setter used for Injection #444

carlosalberto opened this issue Feb 5, 2020 · 7 comments
Assignees
Labels
area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory
Milestone

Comments

@carlosalberto
Copy link
Contributor

From this comment, talking about api-propagators.md:

- the `Setter` invoked for each propagation key to add or remove.

For HTTP, does Setter do header.Set or header.Add?

The W3C tracestate header may be split into multiple entries (repeated headers). It's impossible to do it if Setter only has a Set method.

(But maybe it's ok, since not all transports where we might use HTTPPropagator actually allow repeated headers).

@carlosalberto carlosalberto added this to the Alpha v0.4 milestone Feb 5, 2020
@bogdandrutu bogdandrutu modified the milestones: v0.5, v0.4 Feb 13, 2020
@bogdandrutu bogdandrutu modified the milestones: v0.4, v0.5 May 12, 2020
@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
@arminru arminru added the spec:context Related to the specification/context directory label Jun 9, 2020
@bogdandrutu bogdandrutu added the area:api Cross language API specification issue label Jun 26, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 2, 2020
@mtwo mtwo added this to Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Jul 6, 2020
@carlosalberto carlosalberto added the priority:p1 Highest priority level label Jul 24, 2020
@andrewhsu
Copy link
Member

Having a look at this with @carlosalberto and it looks like this issue may be desirable before the trace api freeze because it could change how Extractions Getter interacts.

@tedsuo
Copy link
Contributor

tedsuo commented Sep 10, 2020

Repeating my comment from #433.

Given that headers can be converted between their single line and multiline equivalents we could require that Set and Get must both use single line representation. Under the hood, the carrier can split and concatenate multiline headers as needed.

A recipient MAY combine multiple header fields with the same field name into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field value to the combined field value in order, separated by a comma.

The exception is Set-Cookie, but I don't believe we should ever be touching that.

@Oberon00
Copy link
Member

Oberon00 commented Sep 11, 2020

Given that headers can be converted between their single line and multiline equivalents

Only if we assume only HTTP will ever be used by the TextMapPropagator
EDIT: Although most other protocols won't support having the same header multiple times, I guess.

@carlosalberto
Copy link
Contributor Author

Given that headers can be converted between their single line and multiline equivalents we could require that Set and Get must both use single line representation.

Would love that myself. If we can agree on that, I'd be super happy.

@carlosalberto
Copy link
Contributor Author

Similarly to #433 I think we should state specifically that Setter does a SET operation, rather than an ADD. Later, if truly needed, we can add an Update method.

cc @yurishkuro (original author of the comment ;) )

@andrewhsu
Copy link
Member

from the spec mtg today, @carlosalberto will add note about details

@carlosalberto
Copy link
Contributor Author

Added #976 to clarify this part of the Specification - the actual addition of an Add operation will be considered after GA.

@carlosalberto carlosalberto added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 21, 2020
@andrewhsu andrewhsu removed this from Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

6 participants