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

Clarify that Setter sets values only. #976

Merged

Conversation

carlosalberto
Copy link
Contributor

Clarify that Setter only sets values - that is, it does not adds values to existing ones.

Related to #444 which aims to (potentially) add an additional Add operation (to added, if needed, after GA).

@Oberon00
Copy link
Member

I would like to see some guidance here specifically for HTTP headers. Say,we have multiple headers with name "Foo" in the request (= carrier) and the propagator calls Set(carrier, "Foo", "bar"). Would that remove all the previous Foo headers and then add that single new one with value "bar"?

@carlosalberto
Copy link
Contributor Author

Would that remove all the previous Foo headers and then add that single new one with value "bar"?

Yes, that's all that a Setter will do at this moment. During our last triage call it was mentioned a Setter is expected to set and override values. This can be explicitly stated in this PR if needed, but if you think we need something else is needed (guidance here specifically for HTTP headers), let's do that in a follow-up PR.

specification/context/api-propagators.md Outdated Show resolved Hide resolved
specification/context/api-propagators.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor Author

@Oberon00 Applied your feedback using slightly different wording. Please review ;)

@carlosalberto
Copy link
Contributor Author

@open-telemetry/specs-approvers Please review this. This is a (hopefully) simple clarification on the Setter functionality ;)

specification/context/api-propagators.md Outdated Show resolved Hide resolved
specification/context/api-propagators.md Outdated Show resolved Hide resolved
carlosalberto and others added 2 commits September 23, 2020 19:23
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto
Copy link
Contributor Author

@arminru Feedback applied, please review.

specification/context/api-propagators.md Show resolved Hide resolved
specification/context/api-propagators.md Show resolved Hide resolved
specification/context/api-propagators.md Outdated Show resolved Hide resolved
specification/context/api-propagators.md Outdated Show resolved Hide resolved
carlosalberto and others added 2 commits September 24, 2020 15:04
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
- A `Setter` invoked for each propagation key to add or remove. This is an additional
argument that languages are free to define to help inject data into the carrier.
- A `Setter` to set a propagation key/value pair. Propagators MAY invoke it multiple times in order to set multiple pairs.
This is an additional argument that languages are free to define to help inject data into the carrier.

#### Setter argument
Copy link
Member

Choose a reason for hiding this comment

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

Actually I am confused by this spec: Does this heading describe the arguments to the setter? Should this be heading be replaced by the sentence: "Inject should have the following parameters:"?

@Oberon00
Copy link
Member

I think we went too often back and forth on this PR and now it does not fulfill the originial intent of specifying that values are overriden/only one value per key is supposed to be retained.

I think the PR is still an improvement, so my approval holds, but are you sure this is what you want @carlosalberto ? I would recommend adding some explicit wording what should happen when the setter is called with the same key multiple times or with a key that is already in the carrier (or is in the carrier multiple times even).

@carlosalberto
Copy link
Contributor Author

To be honest, I'd like to act as @iNikem acted in the past, to avoid scope creep: this PR is doing something and if something else needs to happen, please do that in a follow up. The intention of this PR was simply to avoid using the words "add" and "remove" and use "set" instead.

@carlosalberto carlosalberto added release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:context Related to the specification/context directory labels Sep 25, 2020
@github-actions
Copy link

github-actions bot commented Oct 3, 2020

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 3, 2020
@arminru arminru requested a review from a team October 5, 2020 12:20
@arminru
Copy link
Member

arminru commented Oct 5, 2020

While this PR does not resolve all inconsistencies in this spec, I still think it's an improvement and should be uncontroversial enough to merge if any other @open-telemetry/specs-approvers would take a look.

@github-actions github-actions bot removed the Stale label Oct 6, 2020
@carlosalberto
Copy link
Contributor Author

Merging as this is merely an editorial clean up.

@carlosalberto carlosalberto merged commit 2ebefc1 into open-telemetry:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:context Related to the specification/context directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants