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 SDK Extension for AWS XRay Vendor #130

Merged
merged 6 commits into from
Nov 10, 2020
Merged

Add SDK Extension for AWS XRay Vendor #130

merged 6 commits into from
Nov 10, 2020

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Nov 4, 2020 •
edited
Loading

Description

Following our discussion from the SIG meetings, reopening this PR that was on Core here in the Contrib now that the Contrib repo is much farther along.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Added unit tests to verify expected behavior of these components. They validate the ID generator and Propagator work under multiple scenarios.

Checklist:

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

@NathanielRN NathanielRN requested a review from a team as a code owner November 4, 2020 20:05
@NathanielRN NathanielRN force-pushed the sdk-extension-aws branch 4 times, most recently from b0e30ce to f391cd7 Compare November 5, 2020 06:25
@lzchen lzchen self-assigned this Nov 5, 2020
@NathanielRN NathanielRN closed this Nov 7, 2020
@NathanielRN NathanielRN deleted the sdk-extension-aws branch November 7, 2020 02:04
@NathanielRN NathanielRN reopened this Nov 7, 2020
@NathanielRN NathanielRN force-pushed the sdk-extension-aws branch 2 times, most recently from 6b551f2 to d8faa98 Compare November 7, 2020 03:03
@toumorokoshi
Copy link
Member

IMPORTANT: This PR is waiting on the result of open-telemetry/opentelemetry-specification#1047 (comment)

Can you edit the description to remove this section? it sounds like it was decided that this belongs in contrib.

toumorokoshi
toumorokoshi suggested changes Nov 7, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks! I think as a minimum blocker the version discrepancy should be resolved, but there's also a lot of refactoring that can be done in the extractor in particular to be more python-idiomatic, and more performant as a result.

sdk-extension/opentelemetry-sdk-extension-aws/README.rst Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
sdk-extension/opentelemetry-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/version.py Show resolved Hide resolved
@NathanielRN NathanielRN force-pushed the sdk-extension-aws branch 2 times, most recently from 18c529a to 06c0201 Compare November 7, 2020 08:38
@NathanielRN
Copy link
Contributor Author

@toumorokoshi Thank you so much for taking the time to leave really helpful comments, I'm really happy with the changes :) Please let me know if I addressed them all!

@NathanielRN NathanielRN force-pushed the sdk-extension-aws branch 3 times, most recently from 9b1bfbf to d9a2c8c Compare November 7, 2020 09:17
toumorokoshi
toumorokoshi approved these changes Nov 7, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks! Have a few comments that are still worth looking, but all blocking comments are resolved.

...emetry-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/aws_xray_ids_generator.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
sdk-extension/opentelemetry-sdk-extension-aws/tests/__init__.py Outdated Show resolved Hide resolved
anuraaga
anuraaga reviewed Nov 8, 2020
View reviewed changes
Copy link

@anuraaga anuraaga 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 the work on this! And thanks for the help @toumorokoshi!

...emetry-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/aws_xray_ids_generator.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
...y-sdk-extension-aws/src/opentelemetry/sdk/extension/aws/trace/propagation/aws_xray_format.py Outdated Show resolved Hide resolved
sdk-extension/opentelemetry-sdk-extension-aws/tests/trace/propagation/test_aws_xray_format.py Outdated Show resolved Hide resolved
sdk-extension/opentelemetry-sdk-extension-aws/tests/trace/propagation/test_aws_xray_format.py Outdated Show resolved Hide resolved
sdk-extension/opentelemetry-sdk-extension-aws/tests/trace/propagation/test_aws_xray_format.py Outdated
)

self.assertEqual(
get_extracted_span_context(actual_context_encompassing_extracted),
Copy link

Choose a reason for hiding this comment

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

And similarly, here it's hard to understand the test since it refers to this actual_context_encompassing_extracted and build_test_context, not sure what that is without a lot of jumping

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 changed it up a bit! I decided to keep get_extracted_span_context since it's a 1 line function which helps avoid repeated code.

I split build_test_context into build_test_current_context (to simplify the inject tests) and build_test_span_context because it is highly reused and hopefully easy to understand. I think it is cleaner than having the SpanContext constructor multiple times, but let me know what you think!

@NathanielRN NathanielRN force-pushed the sdk-extension-aws branch 2 times, most recently from e53ca45 to 7dee36c Compare November 8, 2020 22:03
lzchen
lzchen approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! Just some unblocking comments.

@lzchen lzchen merged commit f9eace5 into open-telemetry:master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toumorokoshi toumorokoshi toumorokoshi approved these changes

@lzchen lzchen lzchen approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

Assignees

@lzchen lzchen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@NathanielRN @toumorokoshi @anuraaga @lzchen