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

Converted TextMap propagator getter to a class and added keys method #1196

Merged
merged 21 commits into from
Nov 2, 2020
Merged

Converted TextMap propagator getter to a class and added keys method #1196

merged 21 commits into from
Nov 2, 2020

Conversation

nprajilesh
Copy link
Contributor

@nprajilesh nprajilesh commented Oct 2, 2020 •
edited
Loading

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #1084

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit Tests

Checklist:

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 2, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@nprajilesh nprajilesh changed the title Text map keys Converted TextMap propagator getter to a class and added keys method Oct 2, 2020
@nprajilesh nprajilesh force-pushed the text-map-keys branch 3 times, most recently from 03dae3b to 3886afb Compare October 2, 2020 21:25
@nprajilesh nprajilesh mentioned this pull request Oct 2, 2020
Closed
@nprajilesh nprajilesh marked this pull request as ready for review October 6, 2020 18:36
@nprajilesh nprajilesh requested a review from a team October 6, 2020 18:36
@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Oct 8, 2020
@nprajilesh nprajilesh force-pushed the text-map-keys branch 6 times, most recently from 400d4db to c1e203d Compare October 9, 2020 11:46
@nprajilesh
Copy link
Contributor Author

@lzchen @owais I made code changes to implement it in the above-suggested way but I'm facing some typing errors.

Currently, we have carrier defined as of type TextMapPropagatorT with no type constraints. Running a get() or keys() on this type throws typing errors because those 2 methods are defined for type Dict.

details of build failure - https://github.com/open-telemetry/opentelemetry-python/pull/1196/checks?check_run_id=1231130288

should we constrain the type of TextMapPropagator in the definition? I tried doing this but its throwing errors across multiple files that use the propagator.

@nprajilesh nprajilesh requested a review from owais October 11, 2020 09:50
@nprajilesh nprajilesh requested a review from owais October 25, 2020 15:13
owais
owais reviewed Oct 25, 2020
View reviewed changes
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Looks good. Just need two remaining comments to be resolved.

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py Outdated Show resolved Hide resolved
...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Outdated Show resolved Hide resolved
@nprajilesh nprajilesh requested a review from owais October 25, 2020 16:34
lzchen
lzchen reviewed Oct 26, 2020
View reviewed changes
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
from opentelemetry.trace.status import Status, StatusCanonicalCode


def get_header_from_scope(scope: dict, header_name: str) -> typing.List[str]:
"""Retrieve a HTTP header value from the ASGI scope.
class CarrierGetter(DictGetter):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be extending from Getter instead of DictGetter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzchen Extending DictGetter because of the following reasons:

  • carrier is of type dict
  • there is no keys implementation required here so we can use the default keys implementation in DictGetter
  • if we have to extend the Getter Implementation we have to override the keys() method as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, this getter is more of a specialized implementation of a dict getter so LGTM.

@codeboten codeboten added the rc1 label Oct 29, 2020
codeboten
codeboten reviewed Oct 29, 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.

@owais please review one final time. @nprajilesh it looks like there's some conflicts that need resolving, otherwise this is looking pretty close!

@nprajilesh
Copy link
Contributor Author

@codeboten I have resolved the conflicts.

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@renovate-bot
chore(deps): update dependency sinon to v9 (open-telemetry#1196)
87882cb
lzchen and others added 2 commits November 1, 2020 20:23
@lzchen
Merge branch 'master' into text-map-keys
89393f1
@codeboten
fix a a bad merge conflict resolution
d513961 
tried to fix a merge conflict and accidentally removed the import
codeboten
codeboten approved these changes Nov 2, 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 a big improvement in the code, thanks for implementing it!

@codeboten codeboten merged commit 397f228 into open-telemetry:master Nov 2, 2020
@nprajilesh nprajilesh deleted the text-map-keys branch December 13, 2020 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@codeboten codeboten codeboten approved these changes

@owais owais Awaiting requested review from owais

Assignees

@owais owais

@lzchen lzchen

Labels
release:required-for-ga To be resolved before GA release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Propagation: TextMap propagator getter must provide a keys method
5 participants
@nprajilesh @NathanielRN @owais @codeboten @lzchen