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

update Get function in propagators to combine duplicate keys #1884

Closed
wants to merge 3 commits into from
Closed

update Get function in propagators to combine duplicate keys #1884

wants to merge 3 commits into from
+14 −2

Conversation

tsloughter
Copy link
Member

Fixes #433 and #1870

Changes

This change makes it so the Get function used in TextMapPropagator extraction must combine the values of duplicate keys, a requirement for supporting W3C TraceContext.

@tsloughter tsloughter requested review from a team August 23, 2021 14:48
@tsloughter tsloughter force-pushed the getter-get-duplicates branch from 6067cf6 to eadf7b8 Compare August 23, 2021 14:48
@tsloughter tsloughter mentioned this pull request Aug 23, 2021
Closed
@jkwatson
Copy link
Contributor

Since the Getter implementation is provided by instrumentation, not the API, is this something that we can say MUST to? I'm not sure we can do more than provide strong guidance on this, since we're not in control of all implementations.

@carlosalberto
Copy link
Contributor

? I'm not sure we can do more than provide strong guidance on this

Agreed, same feeling (clear guidance should help a lot, hopefully!)

@tsloughter
Copy link
Member Author

Yea, I guess if being a MUST in the api spec implies that the api has to ensure any Get function passed to the propagator does indeed do this then it isn't something we can require since, unlike giving an interface or type spec, we can't validate it.

But it was already a MUST to give the first entry, so there are likely other places that will need to be updated.

Would changing it to instead saying that the Get function returns the value for a key (removing the existing part about returning the first element) and then noting that the function should do the combining in order to not have potentially unexpected behaviour when used with the builtin propagators?

anuraaga
anuraaga reviewed Aug 24, 2021
View reviewed changes
specification/context/api-propagators.md Outdated
The Get function MUST return the first value of the given propagation key or return null if the key doesn't exist.
The Get function returns the value for a given propagation key or returns null if
the key doesn't exist. If the carrier is a type that allows duplicate keys, like
a list or array as opposed to a dictionary, then `Get` MUST get all the values
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to make sense given that if multiple traceparent come in, they would get concatenated and our parser would fail, satisfying the test case

https://github.com/w3c/trace-context/blob/main/test/test.py#L137

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

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 Sep 1, 2021
@tsloughter
Copy link
Member Author

Sounds like there is agreement so I'll get this updated soon.

@github-actions github-actions bot removed the Stale label Sep 8, 2021
@github-actions
Copy link

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 Sep 16, 2021
@tsloughter tsloughter force-pushed the getter-get-duplicates branch from 38e8faf to 38dc657 Compare September 17, 2021 19:54
@tsloughter
Copy link
Member Author

I've made an update but have not removed the use of MUST. I still can but didn't because I realized it is used in reference to case sensitivity. I'm not sure if both case sensitivity and duplicates should be changed to use SHOULD or if they should remain MUST but I feel they should at least be the same.

@tsloughter tsloughter force-pushed the getter-get-duplicates branch from 38dc657 to 1c8446b Compare September 17, 2021 19:56
@github-actions github-actions bot removed the Stale label Sep 18, 2021
@github-actions
Copy link

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 Sep 25, 2021
@tsloughter
Copy link
Member Author

Ping? I've updated the PR.

@tsloughter tsloughter requested a review from anuraaga September 25, 2021 16:44
@github-actions github-actions bot removed the Stale label Sep 26, 2021
@arminru arminru added area:api Cross language API specification issue spec:context Related to the specification/context directory labels Sep 28, 2021
@mateuszrzeszutek mateuszrzeszutek mentioned this pull request Sep 29, 2021
Merged
@jmacd
Copy link
Contributor

jmacd commented Sep 29, 2021

@tsloughter I think we should leave the MUST for case-sensitivity and change the treatment of duplicates to a SHOULD.

@tsloughter
Copy link
Member Author

@jmacd I don't see how they are different? Both are required to properly handle HTTP headers.

@github-actions
Copy link

github-actions bot commented Oct 7, 2021

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 7, 2021
@tsloughter
Copy link
Member Author

I'd like to get this done with. If I make @jmacd suggested change will I get the necessary approvals to be able to merge it? Even tho I disagree that case sensitivity can be a MUST while combining values is a SHOULD :)

@github-actions github-actions bot removed the Stale label Oct 8, 2021
@jmacd
Copy link
Contributor

jmacd commented Oct 13, 2021

Sorry @tsloughter, I've neglected this PR. :(
I don't feel I have a qualified opinion on this matter.
I would like to re-assign it to a TC member who I think does.

jmacd
jmacd reviewed Oct 13, 2021
View reviewed changes
specification/context/api-propagators.md

If the getter is intended to work with the builtin propagators and the carrier
is a type that allows duplicate keys, like a list or array as opposed to a
dictionary, then the Get function MUST get all the values for the key and
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dictionary, then the Get function MUST get all the values for the key and
dictionary, then the Get function SHOULD get all the values for the key and

@jmacd jmacd assigned yurishkuro and unassigned jmacd Oct 13, 2021
@jmacd
Copy link
Contributor

jmacd commented Oct 13, 2021

@tedsuo would you please provide any thoughts or recommendations for @tsloughter?

@github-actions
Copy link

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 21, 2021
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 28, 2021
@tsloughter
Copy link
Member Author

Damn. It appears I can't reopen an issue?

@tedsuo, @jmacd suggested getting your thoughts/recommendations?

@carlosalberto carlosalberto reopened this Oct 28, 2021
@carlosalberto
Copy link
Contributor

Reopened. Ping @tedsuo ;)

@github-actions github-actions bot removed the Stale label Oct 29, 2021
@github-actions
Copy link

github-actions bot commented Nov 6, 2021

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 Nov 6, 2021
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 13, 2021
@tsloughter
Copy link
Member Author

Ping @tedsuo

@jack-berg jack-berg mentioned this pull request Nov 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmacd jmacd jmacd left review comments

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

@MSNev MSNev MSNev approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

Assignees

@yurishkuro yurishkuro

Labels
area:api Cross language API specification issue spec:context Related to the specification/context directory Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Propagation - Getter handling of multi-value headers/attributes
9 participants
@tsloughter @jkwatson @carlosalberto @jmacd @anuraaga @MSNev @mateuszrzeszutek @yurishkuro @arminru