-
Notifications
You must be signed in to change notification settings - Fork 56
feat: OpenTelemetry trace/spanID integration for Python handlers #889
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
Changes from all commits
c673fae
9958630
48c21b6
12c9884
5869017
2df699e
ba90fd0
4793848
bfc84dc
4189108
64766ed
196749d
3dab0c3
785c837
ff22f7f
8ae8493
7a99cbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
import numbers | ||
import os | ||
import pytest | ||
import sys | ||
import unittest | ||
import uuid | ||
|
||
|
@@ -117,6 +118,25 @@ def setUpModule(): | |
) | ||
|
||
|
||
def _cleanup_otel_sdk_modules(f): | ||
""" | ||
Decorator to delete all references to opentelemetry SDK modules after a | ||
testcase is run. Test case should import opentelemetry SDK modules inside | ||
the function. This is to test situations where the opentelemetry SDK | ||
is not imported at all. | ||
""" | ||
|
||
def wrapped(*args, **kwargs): | ||
f(*args, **kwargs) | ||
|
||
# Deleting from sys.modules should be good enough in this use case | ||
for module_name in list(sys.modules.keys()): | ||
if module_name.startswith("opentelemetry.sdk"): | ||
sys.modules.pop(module_name) | ||
|
||
return wrapped | ||
|
||
|
||
class TestLogging(unittest.TestCase): | ||
JSON_PAYLOAD = { | ||
"message": "System test: test_log_struct", | ||
|
@@ -662,6 +682,43 @@ def test_log_root_handler(self): | |
self.assertEqual(len(entries), 1) | ||
self.assertEqual(entries[0].payload, expected_payload) | ||
|
||
@_cleanup_otel_sdk_modules | ||
def test_log_handler_otel_integration(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to also add a system test without the otel sdk imported? I'm not exactly sure what that should look like, but I want to make sure we have coverage of the situation where otel isn't used at all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to create another file for the newly added system tests, so that the existing TCs aren't importing otel? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you can add an extra file. Or just import within the test functions. Or use a test class. Whatever seems cleanest Does Otel behave any differently based on having the module installed? Or is import state the important part? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually managed to resolve this by creating a decorator that deletes the otel SDK imports after the test case gets run. I don't think it's perfect but it feels good enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok great! Do you have any test cases that test |
||
# Doing OTel imports here to not taint the other tests with OTel SDK imports | ||
from opentelemetry import trace | ||
from opentelemetry.sdk.trace import TracerProvider | ||
|
||
LOG_MESSAGE = "This is a test of OpenTelemetry" | ||
LOGGER_NAME = "otel-integration" | ||
handler_name = self._logger_name(LOGGER_NAME) | ||
|
||
handler = CloudLoggingHandler( | ||
Config.CLIENT, name=handler_name, transport=SyncTransport | ||
) | ||
# only create the logger to delete, hidden otherwise | ||
logger = Config.CLIENT.logger(handler.name) | ||
self.to_delete.append(logger) | ||
|
||
# Set up OTel SDK | ||
provider = TracerProvider() | ||
|
||
tracer = provider.get_tracer("test_system") | ||
with tracer.start_as_current_span("test-span") as span: | ||
context = span.get_span_context() | ||
expected_trace_id = f"projects/{Config.CLIENT.project}/traces/{trace.format_trace_id(context.trace_id)}" | ||
expected_span_id = trace.format_span_id(context.span_id) | ||
expected_tracesampled = context.trace_flags.sampled | ||
|
||
cloud_logger = logging.getLogger(LOGGER_NAME) | ||
cloud_logger.addHandler(handler) | ||
cloud_logger.warning(LOG_MESSAGE) | ||
|
||
entries = _list_entries(logger) | ||
self.assertEqual(len(entries), 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to consider instrumentation source entry here? http://go/cdpe-ops-logentry-changes-source There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whether or not the instrumentation source entry gets added depends on whether or not the global variable
|
||
self.assertEqual(entries[0].trace, expected_trace_id) | ||
self.assertEqual(entries[0].span_id, expected_span_id) | ||
self.assertTrue(entries[0].trace_sampled, expected_tracesampled) | ||
|
||
def test_create_metric(self): | ||
METRIC_NAME = "test-create-metric%s" % (_RESOURCE_ID,) | ||
metric = Config.CLIENT.metric( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no http request data, do we reuse the http_request from last request for otel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no http request data I have it set to null value.