Skip to content

Commit

Permalink
chore(tracing): fix inferred base service for pytest command ran with…
Browse files Browse the repository at this point in the history
… --ddtrace optional argument [backport 2.17] (#11447)

Backport 7297779 from #11394 to 2.17.

# Motiviation

Add special case for `--ddtrace` pytest argument, that was causing CI
tests to have a different service name than local testing. CI testing
adds the `--ddtrace` arg to the command. Previously the code would skip
any args that started with `-` as well as the following arg, but that
should not be the case for this arg.

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: William Conti <[email protected]>
  • Loading branch information
github-actions[bot] and wconti27 authored Nov 19, 2024
1 parent 7256443 commit 0039af8
Show file tree
Hide file tree
Showing 350 changed files with 1,360 additions and 1,271 deletions.
7 changes: 5 additions & 2 deletions ddtrace/contrib/internal/mako/patch.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from ddtrace import config
from ddtrace.constants import SPAN_MEASURED_KEY
from ddtrace.contrib.trace_utils import int_service
from ddtrace.contrib.trace_utils import unwrap as _u
from ddtrace.contrib.trace_utils import wrap as _w
from ddtrace.ext import SpanTypes
Expand All @@ -26,7 +27,7 @@ def patch():
return
mako.__datadog_patch = True

Pin(service=config.service or schematize_service_name("mako")).onto(Template)
Pin().onto(Template)

_w(mako, "template.Template.render", _wrap_render)
_w(mako, "template.Template.render_unicode", _wrap_render)
Expand Down Expand Up @@ -57,7 +58,9 @@ def _wrap_render(wrapped, instance, args, kwargs):
template_name = getattr(instance, "filename", None)
template_name = template_name or DEFAULT_TEMPLATE_NAME

with pin.tracer.trace(func_name(wrapped), pin.service, span_type=SpanTypes.TEMPLATE) as span:
with pin.tracer.trace(
func_name(wrapped), int_service(pin, config.mako, schematize_service_name("mako")), span_type=SpanTypes.TEMPLATE
) as span:
span.set_tag_str(COMPONENT, "mako")

span.set_tag(SPAN_MEASURED_KEY)
Expand Down
11 changes: 9 additions & 2 deletions ddtrace/sampler.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,17 @@ def __init__(self, sample_rate=1.0):
def set_sample_rate(
self,
sample_rate, # type: float
service="", # type: str
env="", # type: str
service=None, # type: Optional[str]
env=None, # type: Optional[str]
):
# type: (...) -> None

# if we have a blank service, we need to match it to the config.service
if service is None:
service = config.service
if env is None:
env = config.env

self._by_service_samplers[self._key(service, env)] = _AgentRateSampler(sample_rate)

def sample(self, span):
Expand Down
4 changes: 3 additions & 1 deletion ddtrace/settings/_inferred_base_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ def detect(self, args: List[str]) -> Optional[ServiceMetadata]:
module_flag = False

for arg in args:
has_flag_prefix = arg.startswith("-")
# we support the --ddtrace option for pytest, and shouldn't skip the following arg
# since it's usually the test location argument.
has_flag_prefix = arg.startswith("-") and not arg.startswith("--ddtrace")
is_env_variable = "=" in arg

should_skip_arg = prev_arg_is_flag or has_flag_prefix or is_env_variable
Expand Down
7 changes: 4 additions & 3 deletions ddtrace/settings/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,15 +474,16 @@ def __init__(self):
self._propagation_http_baggage_enabled = _get_config("DD_TRACE_PROPAGATION_HTTP_BAGGAGE_ENABLED", False, asbool)

self.env = _get_config("DD_ENV", self.tags.get("env"))
self.service = _get_config("DD_SERVICE", self.tags.get("service", DEFAULT_SPAN_SERVICE_NAME))
self.service = _get_config("DD_SERVICE", self.tags.get("service", None))

self._inferred_base_service = detect_service(sys.argv)

if self.service is None and in_gcp_function():
self.service = _get_config(["K_SERVICE", "FUNCTION_NAME"], DEFAULT_SPAN_SERVICE_NAME)
if self.service is None and in_azure_function():
self.service = _get_config("WEBSITE_SITE_NAME", DEFAULT_SPAN_SERVICE_NAME)
if self.service is None and self._inferred_base_service:
self.service = self._inferred_base_service
if self.service is None and DEFAULT_SPAN_SERVICE_NAME:
self.service = _get_config("DD_SERVICE", DEFAULT_SPAN_SERVICE_NAME)

self._extra_services = set()
self._extra_services_queue = None if in_aws_lambda() or not self._remote_config_enabled else File_Queue()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fixes:
- |
ci_visibility: Updates the inferred base service name algorithm to ensure that arguments following ``--ddtrace`` are no
longer skipped when executing tests with pytest. Previously, the algorithm misinterpreted these arguments
as standard flags, overlooking possible test paths that may contribute to the inferred service name.
28 changes: 14 additions & 14 deletions tests/contrib/anthropic/test_anthropic_llmobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def test_completion(self, anthropic, ddtrace_global_config, mock_llmobs_writer,
output_messages=[{"content": 'THE BEST-SELLING BOOK OF ALL TIME IS "DON', "role": "assistant"}],
metadata={"temperature": 0.8, "max_tokens": 15.0},
token_metrics={"input_tokens": 32, "output_tokens": 15, "total_tokens": 47},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -113,7 +113,7 @@ def test_error(self, anthropic, ddtrace_global_config, mock_llmobs_writer, mock_
error_message=span.get_tag("error.message"),
error_stack=span.get_tag("error.stack"),
metadata={"temperature": 0.8, "max_tokens": 15.0},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -142,7 +142,7 @@ def test_error_unserializable_arg(
error_message=span.get_tag("error.message"),
error_stack=span.get_tag("error.stack"),
metadata={"temperature": 0.8, "max_tokens": mock.ANY},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
mock_llmobs_writer.enqueue.assert_called_with(expected_span)
actual_span = mock_llmobs_writer.enqueue.call_args[0][0]
Expand Down Expand Up @@ -196,7 +196,7 @@ def test_stream(self, anthropic, ddtrace_global_config, mock_llmobs_writer, mock
],
metadata={"temperature": 0.8, "max_tokens": 15.0},
token_metrics={"input_tokens": 27, "output_tokens": 15, "total_tokens": 42},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -253,7 +253,7 @@ def test_stream_helper(self, anthropic, ddtrace_global_config, mock_llmobs_write
],
metadata={"temperature": 0.8, "max_tokens": 15.0},
token_metrics={"input_tokens": 27, "output_tokens": 15, "total_tokens": 42},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -308,7 +308,7 @@ def test_image(self, anthropic, ddtrace_global_config, mock_llmobs_writer, mock_
],
metadata={"temperature": 0.8, "max_tokens": 15.0},
token_metrics={"input_tokens": 246, "output_tokens": 15, "total_tokens": 261},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -350,7 +350,7 @@ def test_tools_sync(self, anthropic, ddtrace_global_config, mock_llmobs_writer,
],
metadata={"max_tokens": 200.0},
token_metrics={"input_tokens": 599, "output_tokens": 152, "total_tokens": 751},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -403,7 +403,7 @@ def test_tools_sync(self, anthropic, ddtrace_global_config, mock_llmobs_writer,
],
metadata={"max_tokens": 500.0},
token_metrics={"input_tokens": 768, "output_tokens": 29, "total_tokens": 797},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -446,7 +446,7 @@ async def test_tools_async(self, anthropic, ddtrace_global_config, mock_llmobs_w
],
metadata={"max_tokens": 200.0},
token_metrics={"input_tokens": 599, "output_tokens": 152, "total_tokens": 751},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -499,7 +499,7 @@ async def test_tools_async(self, anthropic, ddtrace_global_config, mock_llmobs_w
],
metadata={"max_tokens": 500.0},
token_metrics={"input_tokens": 768, "output_tokens": 29, "total_tokens": 797},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -559,7 +559,7 @@ def test_tools_sync_stream(self, anthropic, ddtrace_global_config, mock_llmobs_w
],
metadata={"max_tokens": 200.0},
token_metrics={"input_tokens": 599, "output_tokens": 135, "total_tokens": 734},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -609,7 +609,7 @@ def test_tools_sync_stream(self, anthropic, ddtrace_global_config, mock_llmobs_w
],
metadata={"max_tokens": 500.0},
token_metrics={"input_tokens": 762, "output_tokens": 33, "total_tokens": 795},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -664,7 +664,7 @@ async def test_tools_async_stream_helper(
],
metadata={"max_tokens": 200.0},
token_metrics={"input_tokens": 599, "output_tokens": 146, "total_tokens": 745},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)

Expand Down Expand Up @@ -719,6 +719,6 @@ async def test_tools_async_stream_helper(
],
metadata={"max_tokens": 500.0},
token_metrics={"input_tokens": 762, "output_tokens": 18, "total_tokens": 780},
tags={"ml_app": "<ml-app-name>"},
tags={"ml_app": "<ml-app-name>", "service": "tests.contrib.anthropic"},
)
)
4 changes: 2 additions & 2 deletions tests/contrib/django/test_django.py
Original file line number Diff line number Diff line change
Expand Up @@ -1425,14 +1425,14 @@ def test_cached_view(client, test_spans):
"django.cache.key": (
"views.decorators.cache.cache_page..GET.03cdc1cc4aab71b038a6764e5fcabb82.d41d8cd98f00b204e9800998ecf8..."
),
"_dd.base_service": "",
"_dd.base_service": "tests.contrib.django",
}

expected_meta_header = {
"component": "django",
"django.cache.backend": "django.core.cache.backends.locmem.LocMemCache",
"django.cache.key": "views.decorators.cache.cache_header..03cdc1cc4aab71b038a6764e5fcabb82.en-us",
"_dd.base_service": "",
"_dd.base_service": "tests.contrib.django",
}

assert span_view.get_tags() == expected_meta_view
Expand Down
4 changes: 2 additions & 2 deletions tests/contrib/flask/test_blueprint.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def test():
self.assertEqual(span.name, "bp.test")
self.assertEqual(span.resource, "/")
self.assertNotEqual(span.parent_id, 0)
self.assertEqual(span.get_tags(), {"component": "flask", "_dd.base_service": ""})
self.assertEqual(span.get_tags(), {"component": "flask", "_dd.base_service": "tests.contrib.flask"})

def test_blueprint_request_pin_override(self):
"""
Expand Down Expand Up @@ -132,7 +132,7 @@ def test():
self.assertEqual(span.name, "bp.test")
self.assertEqual(span.resource, "/")
self.assertNotEqual(span.parent_id, 0)
self.assertEqual(span.get_tags(), {"component": "flask", "_dd.base_service": ""})
self.assertEqual(span.get_tags(), {"component": "flask", "_dd.base_service": "tests.contrib.flask"})

def test_blueprint_request_pin_disabled(self):
"""
Expand Down
2 changes: 1 addition & 1 deletion tests/contrib/flask/test_errorhandler.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from . import BaseFlaskTestCase


EXPECTED_METADATA = {"component": "flask", "_dd.base_service": ""}
EXPECTED_METADATA = {"component": "flask", "_dd.base_service": "tests.contrib.flask"}


class FlaskErrorhandlerTestCase(BaseFlaskTestCase):
Expand Down
2 changes: 1 addition & 1 deletion tests/contrib/flask/test_flask_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def test_jsonify(self):
spans = self.get_spans()
self.assertEqual(len(spans), 3)

self.assertIsNone(spans[0].service)
self.assertEqual(spans[0].service, "tests.contrib.flask")
self.assertEqual(spans[0].name, "flask.jsonify")
self.assertEqual(spans[0].resource, "flask.jsonify")
assert set(spans[0].get_tags().keys()) == {"runtime-id", "_dd.p.dm", "_dd.p.tid", "component", "language"}
Expand Down
4 changes: 2 additions & 2 deletions tests/contrib/flask/test_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def test_render_template(self):
spans = self.get_spans()
self.assertEqual(len(spans), 3)

self.assertIsNone(spans[0].service)
self.assertEqual(spans[0].service, "tests.contrib.flask")
self.assertEqual(spans[0].name, "flask.render_template")
resource = "tests.contrib.flask" if flask_version >= (2, 2, 0) else "test.html"
self.assertEqual(spans[0].resource, resource) # FIXME: should always be 'test.html'?
Expand Down Expand Up @@ -92,7 +92,7 @@ def test_render_template_string(self):
spans = self.get_spans()
self.assertEqual(len(spans), 3)

self.assertIsNone(spans[0].service)
self.assertEqual(spans[0].service, "tests.contrib.flask")
self.assertEqual(spans[0].name, "flask.render_template_string")
resource = "tests.contrib.flask" if flask_version >= (2, 2, 0) else "<memory>"
self.assertEqual(spans[0].resource, resource) # FIXME: should always be '<memory>'?
Expand Down
2 changes: 1 addition & 1 deletion tests/contrib/flask/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

base_exception_name = "builtins.Exception"

EXPECTED_METADATA = {"component": "flask", "_dd.base_service": ""}
EXPECTED_METADATA = {"component": "flask", "_dd.base_service": "tests.contrib.flask"}


class FlaskViewTestCase(BaseFlaskTestCase):
Expand Down
Loading

0 comments on commit 0039af8

Please sign in to comment.