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

Add fields property #1374

Merged
merged 9 commits into from
Dec 2, 2020
Merged

Add fields property #1374

merged 9 commits into from
Dec 2, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Nov 11, 2020 •
edited
Loading

Fixes #1104

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • test_fields

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Tagging @carlosalberto as requested 👍

@ocelotl ocelotl requested review from a team, codeboten and hectorhdzg and removed request for a team November 11, 2020 01:43
@ocelotl ocelotl self-assigned this Nov 11, 2020
@ocelotl ocelotl added api Affects the API package. sdk Affects the SDK package. labels Nov 11, 2020
@ocelotl ocelotl force-pushed the issue_1104 branch 10 times, most recently from cba1c06 to 08d86fb Compare November 11, 2020 03:16
codeboten
codeboten suggested changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! Just a couple of questions I'd like to have answered before I approve

tato.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/propagators/composite.py

for propagator in self._propagators:
for field in propagator.fields:
composite_fields.add(field)
Copy link
Contributor

Choose a reason for hiding this comment

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

is the behaviour for the composite propagator specified in the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is, I tried to replicate the Go implementation with one particular difference. In this implementation a set is returned instead of a list. There is no defined order for these fields (these fields may be called in inject in an arbitrary order and even if this order is constant it has no relation with the fields themselves), so an ordered sequence like a list makes no sense. Also, (if I understand correctly) the intention of this attribute is to allow the user to tell if a certain field is also used in inject. That is an in operation that has a better (O(1)) performance in a set than in a list (O(N)).

opentelemetry-api/src/opentelemetry/trace/propagation/tracecontext.py Show resolved Hide resolved
@lzchen lzchen self-assigned this Nov 12, 2020
@ocelotl ocelotl force-pushed the issue_1104 branch 2 times, most recently from 19ee9fa to 7d432b9 Compare November 13, 2020 04:50
@ocelotl ocelotl requested a review from codeboten November 13, 2020 05:09
@ocelotl ocelotl force-pushed the issue_1104 branch 2 times, most recently from 579af1a to 92ccbf1 Compare November 13, 2020 19:19
@codeboten codeboten assigned codeboten and unassigned ocelotl and lzchen Nov 19, 2020
codeboten
codeboten approved these changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments.

@codeboten codeboten added the rc1 label Nov 25, 2020
@ocelotl ocelotl mentioned this pull request Nov 30, 2020
Closed
@ocelotl ocelotl force-pushed the issue_1104 branch 2 times, most recently from 1ff26a1 to c76a8f9 Compare December 1, 2020 00:23
NathanielRN
NathanielRN reviewed Dec 1, 2020
View reviewed changes
opentelemetry-api/CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## Unreleased

- Add `fields` to propagators ([#1374](https://github.com/open-telemetry/opentelemetry-python/pull/1374))
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR link should probably go on a new line

@NathanielRN NathanielRN mentioned this pull request Dec 2, 2020
Merged
11 tasks
@lzchen lzchen merged commit 50f5c6c into open-telemetry:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanielRN NathanielRN NathanielRN left review comments

@lzchen lzchen lzchen approved these changes

@codeboten codeboten codeboten approved these changes

@hectorhdzg hectorhdzg Awaiting requested review from hectorhdzg

Assignees

@codeboten codeboten

Labels
api Affects the API package. sdk Affects the SDK package.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Propagation: TextMap propagator must provide predefined propagation fields
4 participants
@ocelotl @codeboten @lzchen @NathanielRN