Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

In Jaeger Config, enable to add directly a sampler #324

Merged
merged 4 commits into from
Sep 3, 2021
Merged

In Jaeger Config, enable to add directly a sampler #324

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e51936c
In Jaeger Config, enable to add directly a sampler
Sep 3, 2021
eb2b748
Fix tests
Sep 3, 2021
0afa43b
PEP-8
Sep 3, 2021
a58551d
Come back to `sampler_config`
Etienne-Carriere Sep 3, 2021
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
10 changes: 6 additions & 4 deletions jaeger_client/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
ProbabilisticSampler,
RateLimitingSampler,
RemoteControlledSampler,
)
Sampler)
from .constants import (
DEFAULT_SAMPLING_INTERVAL,
DEFAULT_FLUSH_INTERVAL,
Expand Down Expand Up @@ -219,9 +219,11 @@ def max_traceback_length(self):

@property
def sampler(self):
sampler_config = self.config.get('sampler', {})
sampler_type = sampler_config.get('type', None)
sampler_param = sampler_config.get('param', None)
sampler_param = self.config.get('sampler', {})
Copy link
Member

Choose a reason for hiding this comment

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

this is overloading sampler_param variable. I would keep it as sampler_config

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 choose to change from sampler_config to sampler_param as now it can be both a sampler or a config so I choose a more generic name but if you want I can come back to sampler_config

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with sampler_config or sampler (prefer config as it's more common use case). But param is confusing since it's the inner field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right . I come back to sampler_config .

if isinstance(sampler_param, Sampler):
return sampler_param
sampler_type = sampler_param.get('type', None)
sampler_param = sampler_param.get('param', None)
if not sampler_type:
return None
elif sampler_type == SAMPLER_TYPE_CONST:
Expand Down
11 changes: 10 additions & 1 deletion tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,18 @@
# limitations under the License.

from __future__ import absolute_import

import os
import unittest

import opentracing.tracer

from jaeger_client import Config, ConstSampler, ProbabilisticSampler, RateLimitingSampler
from jaeger_client import constants
from jaeger_client.config import DEFAULT_THROTTLER_PORT
from jaeger_client.metrics import MetricsFactory
from jaeger_client.reporter import NullReporter
from jaeger_client import constants
from tests.test_utils import TestSampler


class ConfigTests(unittest.TestCase):
Expand Down Expand Up @@ -80,6 +84,11 @@ def test_bad_sampler(self):
with self.assertRaises(ValueError):
c.sampler.is_sampled(0)

def test_object_sampler_sampler(self):
sampler = TestSampler()
c = Config({'sampler': sampler}, service_name='x')
assert c.sampler is sampler

def test_agent_reporting_host(self):
c = Config({}, service_name='x')
assert c.local_agent_reporting_host == 'localhost'
Expand Down
13 changes: 9 additions & 4 deletions tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import unittest

from jaeger_client import utils
from jaeger_client.sampler import Sampler


class ConfigTests(unittest.TestCase):
Expand All @@ -43,10 +44,10 @@ def test_get_unknown_boolean(self):
def test_get_None_boolean(self):
self.check_boolean(None, 'qwer', False)

# def test_error_reporter_doesnt_send_metrics_if_not_configured(self):
# er = utils.ErrorReporter(False)
# er.error('foo', 1)
# assert not mock_metrics.count.called
# def test_error_reporter_doesnt_send_metrics_if_not_configured(self):
# er = utils.ErrorReporter(False)
# er.error('foo', 1)
# assert not mock_metrics.count.called

def test_error_reporter_sends_metrics_if_configured(self):
mock_metrics = mock.MagicMock()
Expand Down Expand Up @@ -81,3 +82,7 @@ def test_local_ip_does_not_blow_up():
def test_get_local_ip_by_socket_does_not_blow_up():
import jaeger_client.utils
jaeger_client.utils.get_local_ip_by_socket()


class TestSampler(Sampler):
pass