Khanayan123/add consistent config system tests #3745
Conversation
khanayan123
requested review from
christophe-papazian,
manuel-alvarez-alvarez,
jandro996 and
cataphract
and removed request for
a team
manifests/cpp.yml
Outdated
| @@ -241,8 +241,14 @@ tests/: | |||
| Test_Config_HttpServerErrorStatuses_FeatureFlagCustom: missing_feature | |||
| Test_Config_IntegrationEnabled_False: missing_feature | |||
| Test_Config_IntegrationEnabled_True: missing_feature | |||
| Test_Config_LogInjection_128Bit_TradeId_Default: missing_feature | |||
There was a problem hiding this comment.
Instead of missing feature can we use
system-tests/docs/edit/skip-tests.md
Line 5 in 25c79a3
| @missing_feature(library="cpp", reason="Not implemented") | ||
| @missing_feature(library="nodejs", reason="Not implemented") |
| ], | ||
| ) | ||
| def test_trace_sampling_rules_format_not_needed(self, test_agent, test_library): | ||
| """Glob should work by default without specifying the format""" |
There was a problem hiding this comment.
The assertions/spans in this test look identical to test_field_case_insensitivity. S
hould we add the library_env in this test to test_field_case_insensitivity parameterized test? This way we have one test with 5 scenarios.
There was a problem hiding this comment.
I could just remove it from all the tests since I believe no language currently requires the glob environment variable
tests/test_config_consistency.py
Outdated
|
|
||
| def test_log_injection_enabled(self): | ||
| assert self.r.status_code == 200 | ||
| pattern = rf'"dd":\{{"trace_id":"[^"]+","span_id":"\d+","service":"[^"]+","version":"[^"]+","env":"[^"]+"\}},"msg":"{self.message}"' |
There was a problem hiding this comment.
Should we make this pattern more permissive? If there's whitespace between the entries then the pattern will fail to match. Also it requires log correlation fields to be wrapped in curly brackets and the message to printed after a comma. Also the values may not always be wrapped in quotes.
This is an example of a log message generated by the python tracer:
Python docs: https://docs.datadoghq.com/tracing/other_telemetry/connect_logs_and_traces/python/
import ddtrace
import logging
FORMAT = ('%(asctime)s %(levelname)s [%(name)s] [%(filename)s:%(lineno)d] '
'[dd.service=%(dd.service)s dd.env=%(dd.env)s dd.version=%(dd.version)s dd.trace_id=%(dd.trace_id)s dd.span_id=%(dd.span_id)s] '
'- %(message)s')
logging.basicConfig(format=FORMAT)
log = logging.getLogger(__name__)
with ddtrace.tracer.trace("test"):
log.error("test")
Output:
2025-01-06 17:21:00,421 ERROR [__main__] [reproduction.py:20] [dd.service=Downloads dd.env= dd.version= dd.trace_id=677c574c000000001679fe710d0f33ba dd.span_id=10973182204132464872] - test
There was a problem hiding this comment.
I think the other way to solve this issue is to have weblog apps send json formatted logs
There was a problem hiding this comment.
That would make parsing easier
| @@ -554,3 +517,19 @@ class Test_Config_RuntimeMetrics_Default: | |||
| def test_config_runtimemetrics_default(self): | |||
| data = list(interfaces.library.get_data("/dogstatsd/v2/proxy")) | |||
| assert len(data) == 0 | |||
|
|
|||
| def parse_log_injection_message(log_message): | |||
There was a problem hiding this comment.
Could you document a little bit the object of this function?
tests/test_config_consistency.py
Outdated
| json_string = json.dumps(data) | ||
| parsed_data = json.loads(json_string) |
There was a problem hiding this comment.
I don't get the purpose of those two line 🤔
tests/test_config_consistency.py
Outdated
| class Test_Config_RuntimeMetrics_Enabled: | ||
| # This test verifies runtime metrics by asserting the prescene of a metric in the dogstatsd endpoint | ||
| def test_config_runtimemetrics_enabled(self): | ||
| data = list(interfaces.library.get_data("/dogstatsd/v2/proxy"))[0] |
There was a problem hiding this comment.
There might be a race condition here. At least in .NET we submit runtime metrics every 10s, so we'll need to add some sort of wait or configure the delay in a similar fashion for both the Enabled case and the Default case
Motivation
Adds new system tests to assert config consistency
Changes
Adds system tests for DD_TRACE_SAMPLING_RULES, DD_LOG_INEJCTION, DD_RUNTIME_METRICS_ENABLED, & DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED, DD_TRACE_AGENT_URL, as per https://docs.google.com/document/d/1kI-gTAKghfcwI7YzKhqRv2ExUstcHqADIWA4-TZ387o/edit?usp=sharing
DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED tests were tested against this PR:
DataDog/dd-trace-js#5021