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

Added the option to ignore missing segments #338

Merged
merged 6 commits into from
May 31, 2022

Conversation

stijndehaes
Copy link
Contributor

Issue #, if available: #334

Description of changes:

Adds the option to ignore a missing context

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@NathanielRN
Copy link
Contributor

Normally we have tests follow alongside the change, but I can't think of any easy way to check if a log was outputted to the console.

Otherwise the test could live next to this one:

def test_capture_not_swallow_return():
xray_recorder = get_new_stubbed_recorder()
xray_recorder.configure(sampling=False, context_missing='LOG_ERROR')
value = 1
@xray_recorder.capture()
def my_func():
return value
actual = my_func()
assert actual == value

Let me know if you know how we could test for log generation? I was checking out this SO post but unfortunately we don't use unittest.TestCase in our file. Although that would be a nice improvement 🙂

I'll try to think of a way but if I can't I'll come back and update on this PR 🙂

@heitorlessa
Copy link

heitorlessa commented May 25, 2022 via email

@stijndehaes
Copy link
Contributor Author

I added a test for the ignore. However I am having a lot of trouble running the tests. I tried to run the command tox. But getting lot's of invation errors. Here are some examples:

ERROR: invocation failed (exit code 1), logfile: /home/sdehaes/opensource/aws-xray-sdk-python/.tox/py37-ext-sqlalchemy_core/log/py37-ext-sqlalchemy_core-0.log
ERROR: InvocationError for command /usr/bin/python3 -m virtualenv --no-download --python /usr/bin/python3.7 py37-ext-sqlalchemy_core (exited with code 1)
ERROR: invocation failed (exit code 1), logfile: /home/sdehaes/opensource/aws-xray-sdk-python/.tox/py38-ext-sqlalchemy_core/log/py38-ext-sqlalchemy_core-1.log

Any idea what might be wrong? I am running ubuntu in WSL2

@stijndehaes stijndehaes force-pushed the feature/add-ignore-support branch from 552c73d to 07f5b32 Compare May 26, 2022 09:32
@stijndehaes
Copy link
Contributor Author

You can use the built-in capsys test fixture by pytest. By adding “capsys” in the test function argument, all log generated will be automatically captured and you can assert on those. https://docs.pytest.org/en/6.2.x/capture.html

On Wed, 25 May 2022 at 01:04, Nathaniel Ruiz Nowell < @.> wrote: Normally we have tests follow alongside the change, but I can't think of any easy way to check if a log was outputted to the console. Otherwise the test could live next to this one:

def test_capture_not_swallow_return():
xray_recorder = get_new_stubbed_recorder()
xray_recorder.configure(sampling=False, context_missing='LOG_ERROR')
value = 1
@xray_recorder.capture()
def my_func():
return value
actual = my_func()
assert actual == value
Let me know if you know how we could test for log generation? I was checking out this SO post https://stackoverflow.com/a/34920727 but unfortunately we don't use unittest.TestCase in our file. Although that would be a nice improvement 🙂 I'll try to think of a way but if I can't I'll come back and update on this PR 🙂 — Reply to this email directly, view it on GitHub <#338 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZPQBHHYF6NDTGLIKFVLQLVLVOAPANCNFSM5WZUFLFQ . You are receiving this because you are subscribed to this thread.Message ID: @.>

I instead used caplog which is meant to capture logs :) Got the tests working by enabling github actions on my fork. So everything should be fine now!

Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

Thank you for this!

Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

Ah so sorry I forgot to call this out earlier, can you add this change under the CHANGELOG.rst: https://github.com/aws/aws-xray-sdk-python/blob/master/CHANGELOG.rst

Under an “Unreleased” heading please? Thank you!

@stijndehaes
Copy link
Contributor Author

@NathanielRN
The option is called 'IGNORE_ERROR' both in the java sdk and golang sdk.

Perhaps I should change it to that?

@NathanielRN
Copy link
Contributor

@stijndehaes Yes please! I would greatly appreciate that since it will make our doc writers’ lives easier 🙂 Thank you! 😄

This is the same naming used in other SDK's
This is the same naming used in other SDK's
@stijndehaes stijndehaes requested a review from NathanielRN May 31, 2022 08:34
Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

Thank you so much, @stijndehaes! Very grateful to this community for helping to make this SDK better 😄

@NathanielRN NathanielRN merged commit 816ab26 into aws:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide IGNORE option for AWS_XRAY_CONTEXT_MISSING configuration variable
3 participants