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

Allow configuring different Sampler in Django App #252

Merged
merged 3 commits into from
Dec 7, 2020
Merged

Allow configuring different Sampler in Django App #252

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c77c755
Allow configuring different Sampler in Django App
lalvarezguillen Nov 26, 2020
5e35333
Merge branch 'master' into master
srprash Dec 2, 2020
04494e5
Merge branch 'master' into master
srprash Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aws_xray_sdk/ext/django/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def ready(self):
daemon_address=settings.AWS_XRAY_DAEMON_ADDRESS,
sampling=settings.SAMPLING,
sampling_rules=settings.SAMPLING_RULES,
sampler=settings.SAMPLER,
context_missing=settings.AWS_XRAY_CONTEXT_MISSING,
plugins=settings.PLUGINS,
service=settings.AWS_XRAY_TRACING_NAME,
Expand Down
1 change: 1 addition & 0 deletions aws_xray_sdk/ext/django/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
'PLUGINS': (),
'SAMPLING': True,
'SAMPLING_RULES': None,
'SAMPLER': None,
'AWS_XRAY_TRACING_NAME': None,
'DYNAMIC_NAMING': None,
'STREAMING_THRESHOLD': None,
Expand Down
2 changes: 2 additions & 0 deletions tests/ext/django/app/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Config file for a django app used by django testing client
"""
import os
from aws_xray_sdk.core.sampling.sampler import LocalSampler


BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
Expand Down Expand Up @@ -60,6 +61,7 @@
XRAY_RECORDER = {
'AWS_XRAY_TRACING_NAME': 'django',
'SAMPLING': False,
'SAMPLER': LocalSampler(),
}

LANGUAGE_CODE = 'en-us'
Expand Down
16 changes: 16 additions & 0 deletions tests/ext/django/test_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from unittest import mock

import django
from aws_xray_sdk import global_sdk_config
from django.test import TestCase, override_settings
from django.conf import settings
from django.apps import apps

from aws_xray_sdk.core import xray_recorder
from aws_xray_sdk.core.sampling.sampler import LocalSampler


class XRayConfigurationTestCase(TestCase):
def test_sampler_can_be_configured(self):
assert isinstance(settings.XRAY_RECORDER['SAMPLER'], LocalSampler)
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 would have loved to overwrite settings.XRAY_RECORDER['SAMPLER'] here, to show the recorder working with different samplers.

But because xray_recorder is configured in the ready method of the app, and the ready method is only called once when the test suite is run, that's not easy to implement (cleanly) currently. (you can read more about it here, or verify with a pdb.set_trace() on the ready method)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is way to test different samplers by having different django settings and have it loaded using settings.configure(default_settings=settings_with_LocalSampler) and then calling django.setup() for each test case. More info here. But I agree with you that it may not be clean. I think this is good for now since it tests what we want at this point.

assert isinstance(xray_recorder.sampler, LocalSampler)