Repository: lightstep/lightstep-tracer-python

Issue:
Add the tag `lightstep.hostname`
Hola crew! 

I would like to propose the tag 'lightstep.hostname' is appended to this library. It is present in the Go and JavaScript libraries, and would be useful such that we do not have to append that tag to the tracing constructor manually.

See:

- https://github.com/lightstep/lightstep-tracer-go/blob/eb40dd366f0f992fd08eb681d18c3898fcd68a64/options.go#L41
- https://github.com/lightstep/lightstep-tracer-javascript/blob/e9db0768cf7f3e063b738da479d734844092311e/src/imp/runtime_imp.js#L57

The practical value is to allow us to identify sick nodes for inspection. Whether it is the nodes themselves that are sick, or whether it is some condition about those nodes that is otherwise invisible to the trace (i.e. AZ, connection state or so) -- it's still useful to be able to see an identity to start the inspection.

Discussion:

Patch:
diff --git a/lightstep/http_converter.py b/lightstep/http_converter.py
index c17573d..a05c582 100644
--- a/lightstep/http_converter.py
+++ b/lightstep/http_converter.py
@@ -1,8 +1,10 @@
+import socket
+import sys
+
 from lightstep.collector_pb2 import Auth, ReportRequest, Span, Reporter, KeyValue, Reference, SpanContext
 from lightstep.converter import Converter
 from . import util
 from . import version as tracer_version
-import sys
 from google.protobuf.timestamp_pb2 import Timestamp
 
 
@@ -22,6 +24,7 @@ class HttpConverter(Converter):
         if tags is None:
             tags = {}
         tracer_tags = tags.copy()
+        tracer_tags['lightstep.hostname'] = tracer_tags.get('lightstep.hostname', socket.gethostname())
 
         tracer_tags.update({
             'lightstep.tracer_platform': 'python',
diff --git a/lightstep/thrift_converter.py b/lightstep/thrift_converter.py
index ffcf314..72debc5 100644
--- a/lightstep/thrift_converter.py
+++ b/lightstep/thrift_converter.py
@@ -1,7 +1,9 @@
+import socket
+import sys
+
 from lightstep import constants
 from lightstep.converter import Converter
 from .crouton import ttypes
-import sys
 from . import util
 from . import version as tracer_version
 import jsonpickle
@@ -22,6 +24,7 @@ class ThriftConverter(Converter):
         if tags is None:
             tags = {}
         tracer_tags = tags.copy()
+        tracer_tags['lightstep.hostname'] = tracer_tags.get('lightstep.hostname', socket.gethostname())
 
         tracer_tags.update({
             'lightstep.tracer_platform': 'python',


Test patch:
diff --git a/tests/recorder_test.py b/tests/recorder_test.py
index c514aba..b8d4163 100644
--- a/tests/recorder_test.py
+++ b/tests/recorder_test.py
@@ -1,3 +1,4 @@
+import socket
 import time
 import unittest
 
@@ -15,6 +16,7 @@ from lightstep.crouton import ttypes
 class MockConnection(object):
     """MockConnection is used to debug and test Runtime.
     """
+
     def __init__(self):
         self.reports = []
         self.ready = False
@@ -40,121 +42,140 @@ class MockConnection(object):
 
 @pytest.fixture(params=[True, False])
 def recorder(request):
-    runtime_args = {'collector_encryption': 'none',
-                         'collector_host': 'localhost',
-                         'collector_port': 9998,
-                         'access_token': '{your_access_token}',
-                         'component_name': 'python/runtime_test',
-                         'periodic_flush_seconds': 0,
-                         'use_thrift': request.param,
-                         'use_http': not request.param}
-    recorder = lightstep.recorder.Recorder(runtime_args)
-    yield recorder
-
-
-# ------_
-# HELPERS
-# ------_
-def check_spans(converter, report):
-    """Checks spans' name.
-    """
-    def setUp(self):
-        self.mock_connection = MockConnection()
-        self.mock_connection.open()
-        self.runtime_args = {'collector_encryption': 'none',
-                             'collector_host': 'localhost',
-                             'collector_port': 9998,
-                             'access_token': '{your_access_token}',
-                             'component_name': 'python/runtime_test',
-                             'periodic_flush_seconds': 0}
-
-    def create_test_recorder(self):
-        """Returns a LightStep Recorder based on self.runtime_args.
-        """
-        return lightstep.recorder.Recorder(**self.runtime_args)
-
-    # -------------
-    # SHUTDOWN TESTS
-    # -------------
-    def test_send_spans_after_shutdown(self):
-        recorder = self.create_test_recorder()
-
-        # Send 10 spans
-        for i in range(10):
-            recorder.record_span(self.dummy_basic_span(recorder, i))
-        self.assertTrue(recorder.flush(self.mock_connection))
-
-        # Check 10 spans
-        self.check_spans(self.mock_connection.reports[0].span_records)
-
-        # Delete current logs and shutdown runtime
-        self.mock_connection.clear()
-        recorder.shutdown()
-
-        # Send 10 spans, though none should get through
-        for i in range(10):
-            recorder.record_span(self.dummy_basic_span(recorder, i))
-        self.assertFalse(recorder.flush(self.mock_connection))
-        self.assertEqual(len(self.mock_connection.reports), 0)
-
-    def test_shutdown_twice(self):
-        recorder = self.create_test_recorder()
+    runtime_args = {
+        "collector_encryption": "none",
+        "collector_host": "localhost",
+        "collector_port": 9998,
+        "access_token": "{your_access_token}",
+        "component_name": "python/runtime_test",
+        "periodic_flush_seconds": 0,
+        "use_thrift": request.param,
+        "use_http": not request.param,
+    }
+    yield lightstep.recorder.Recorder(**runtime_args)
+
+
+def test_default_tags_set_correctly(recorder):
+    mock_connection = MockConnection()
+    mock_connection.open()
+    tags = getattr(recorder._runtime, "tags", None)
+    if tags is None:
+        tags = getattr(recorder._runtime, "attrs")
+    for tag in tags:
+        if hasattr(tag, "key"):
+            if tag.key == "lightstep.hostname":
+                assert tag.string_value == socket.gethostname()
+            elif tag.key == "lightstep.tracer_platform":
+                assert tag.string_value == "python"
+        else:
+            if tag.Key == "lightstep.hostname":
+                assert tag.Value == socket.gethostname()
+            elif tag.Key == "lightstep.tracer_platform":
+                assert tag.Value == "python"
+    assert len(tags) == 6
+    runtime_args = {
+        "collector_encryption": "none",
+        "collector_host": "localhost",
+        "collector_port": 9998,
+        "access_token": "{your_access_token}",
+        "component_name": "python/runtime_test",
+        "periodic_flush_seconds": 0,
+        "tags": {
+            "lightstep.hostname": "hostname",
+        },
+    }
+    new_recorder = lightstep.recorder.Recorder(**runtime_args)
+    for tag in new_recorder._runtime.tags:
+        if tag.key == "lightstep.hostname":
+            assert tag.string_value == "hostname"
+    assert len(new_recorder._runtime.tags) == 6
+
+
+# --------------
+# SHUTDOWN TESTS
+# --------------
+def test_send_spans_after_shutdown(recorder):
+    mock_connection = MockConnection()
+    mock_connection.open()
+    # Send 10 spans
+    for i in range(10):
+        dummy_basic_span(recorder, i)
+    assert recorder.flush(mock_connection)
+
+    # Check 10 spans
+    check_spans(recorder.converter, mock_connection.reports[0])
+
+    # Delete current logs and shutdown runtime
+    mock_connection.clear()
+    recorder.shutdown()
+
+    # Send 10 spans, though none should get through
+    for i in range(10):
+        recorder.record_span(dummy_basic_span(recorder, i))
+    assert not recorder.flush(mock_connection)
+    assert len(mock_connection.reports) == 0
+
+
+def test_shutdown_twice(recorder):
+    try:
         recorder.shutdown()
         recorder.shutdown()
+    except Exception as error:
+        self.fail("Unexpected exception raised: {}".format(error))
+
+
+# ------------
+# STRESS TESTS
+# ------------
+def test_stress_logs(recorder):
+    mock_connection = MockConnection()
+    mock_connection.open()
+    for i in range(1000):
+        dummy_basic_span(recorder, i)
+    assert recorder.flush(mock_connection)
+    assert recorder.converter.num_span_records(mock_connection.reports[0]) == 1000
+    check_spans(recorder.converter, mock_connection.reports[0])
+
+
+def test_stress_spans(recorder):
+    mock_connection = MockConnection()
+    mock_connection.open()
+    for i in range(1000):
+        dummy_basic_span(recorder, i)
+    assert recorder.flush(mock_connection)
+    assert recorder.converter.num_span_records(mock_connection.reports[0]) == 1000
+    check_spans(recorder.converter, mock_connection.reports[0])
+
+
+# -------------
+# RUNTIME TESTS
+# -------------
+def test_buffer_limits(recorder):
+    mock_connection = MockConnection()
+    mock_connection.open()
+    recorder._max_span_records = 88
+
+    assert len(recorder._span_records) == 0
+    for i in range(0, 100):
+        dummy_basic_span(recorder, i)
+    assert len(recorder._span_records) == 88
+    assert recorder.flush(mock_connection)
 
-    # ------------
-    # STRESS TESTS
-    # ------------
-    def test_stress_logs(self):
-        recorder = self.create_test_recorder()
-        for i in range(1000):
-            recorder.record_span(self.dummy_basic_span(recorder, i))
-        self.assertTrue(recorder.flush(self.mock_connection))
-        self.assertEqual(len(self.mock_connection.reports[0].span_records), 1000)
-        self.check_spans(self.mock_connection.reports[0].span_records)
-
-    def test_stress_spans(self):
-        recorder = self.create_test_recorder()
-        for i in range(1000):
-            recorder.record_span(self.dummy_basic_span(recorder, i))
-        self.assertTrue(recorder.flush(self.mock_connection))
-        self.assertEqual(len(self.mock_connection.reports[0].span_records), 1000)
-        self.check_spans(self.mock_connection.reports[0].span_records)
-
-    # -------------
-    # RUNTIME TESTS
-    # -------------
-
-    def test_buffer_limits(self):
-        self.runtime_args.update({
-            'max_span_records': 88,
-        })
-        recorder = self.create_test_recorder()
-
-        self.assertEqual(len(recorder._span_records), 0)
-        for i in range(0, 10000):
-            recorder.record_span(self.dummy_basic_span(recorder, i))
-        self.assertEqual(len(recorder._span_records), 88)
-        self.assertTrue(recorder.flush(self.mock_connection))
-
-    # ------
-    # HELPER
-    # ------
-    def check_spans(self, spans):
-        """Checks spans' name.
-        """
-        for i, span in enumerate(spans):
-            self.assertEqual(span.span_name, str(i))
-
-    def dummy_basic_span(self, recorder, i):
-        return BasicSpan(
-            lightstep.tracer._LightstepTracer(False, recorder, None),
-            operation_name=str(i),
-            context=SpanContext(
-                trace_id=1000+i,
-                span_id=2000+i),
-            start_time=time.time())
 
-
-if __name__ == '__main__':
-    unittest.main()
+def check_spans(converter, report):
+    """Checks spans' name.
+    """
+    spans = converter.get_span_records(report)
+    for i, span in enumerate(spans):
+        assert converter.get_span_name(span) == str(i)
+
+
+def dummy_basic_span(recorder, i):
+    span = BasicSpan(
+        lightstep.tracer._LightstepTracer(False, recorder, None),
+        operation_name=str(i),
+        context=SpanContext(trace_id=1000 + i, span_id=2000 + i),
+        start_time=time.time() - 100,
+    )
+    span.finish()
+    return span