Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Khanayan123/implement dd trace agent url system test #3073

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Khanayan123/implement dd trace agent url system test #3073

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
33ed196
implement system test for dd_trace_agent_url
khanayan123 Sep 19, 2024
afedfdf
update manifest files
khanayan123 Sep 19, 2024
fb6172a
run lint
khanayan123 Sep 19, 2024
4c7d51a
fix manifest file ordering
khanayan123 Sep 19, 2024
3712cb6
Update tests/parametric/test_config_consistency.py
khanayan123 Sep 19, 2024
2de9be5
Update tests/parametric/test_config_consistency.py
khanayan123 Sep 19, 2024
c46f5ce
address feedback
khanayan123 Sep 19, 2024
4227910
address feedback and fix typos
khanayan123 Sep 20, 2024
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
1 change: 1 addition & 0 deletions manifests/cpp.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ tests/:
Test_Dsm_Manual_Checkpoint_Intra_Process: missing_feature
parametric/:
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set this to missing_feature? Can we just get this test to pass in this PR OR leave a descriptive message on why this test is failing (ex: missing_feauture (cpp does not support uds)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't necessarily need to give a reason for missing_feature, but it may help later on, so if you have one that is not as obvious as "we didn't started yet to work on this" , feel free to add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mabdinur setting it to missing_feature as of now because the parametric client apps need to be updated to send dd_trace_agent_url

Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in naming:

Suggested change
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceAgentURL: missing_feature

Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/dotnet.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ tests/:
TestAdmisionControllerProfiling: missing_feature
parametric/:
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/golang.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ tests/:
net-http: missing_feature (Endpoint not implemented)
parametric/:
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/java.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,7 @@ tests/:
TestAdmisionControllerProfiling: v1.39.0
parametric/:
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/nodejs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,7 @@ tests/:
TestAdmisionControllerProfiling: *ref_5_22_0
parametric/:
test_config_consistency.py:
Test_Config_DDTraceAgentURL: *ref_5_23_0
Copy link
Member

Choose a reason for hiding this comment

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

was this feature just merged to master ? this version hasn't been released yet

Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/php.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ tests/:
test_128_bit_traceids.py:
Test_128_Bit_Traceids: v0.84.0
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceEnabled: v1.3.0 # Unknown initial version
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/python.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,7 @@ tests/:
test_128_bit_traceids.py:
Test_128_Bit_Traceids: v2.6.0
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
1 change: 1 addition & 0 deletions manifests/ruby.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ tests/:
TestAdmisionControllerProfiling: missing_feature
parametric/:
test_config_consistency.py:
Test_Config_DDTraceAgentURL: missing_feature
Test_Config_TraceEnabled: missing_feature
Test_Config_TraceLogDirectory: missing_feature
Test_Config_UnifiedServiceTagging: missing_feature
Expand Down
40 changes: 40 additions & 0 deletions tests/parametric/test_config_consistency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,43 @@ def test_specific_env(self, library_env, test_agent, test_library):

span = find_span_in_traces(traces, s1.trace_id, s1.span_id)
assert span["meta"]["env"] == "dev"


@scenarios.parametric
@features.tracing_configuration_consistency
class Test_Config_TraceAgentURL:
# DD_TRACE_AGENT_URL is validated using the tracer configuration. This approach avoids the need to modify the setup file to create additional containers at the specified URL, which would be unnecessarily complex.
@parametrize(
"library_env",
[
{
"DD_TRACE_AGENT_URL": "unix:///var/run/datadog/apm.socket",
"DD_AGENT_HOST": "localhost",
"DD_AGENT_PORT": "8126",
}
],
)
def test_dd_trace_agent_unix_url_nonexistent(self, library_env, test_agent, test_library):
with test_library as t:
resp = t.get_tracer_config()
assert resp["dd_trace_agent_url"] == "unix:///var/run/datadog/apm.socket"
with pytest.raises(ValueError):
test_agent.wait_for_num_traces(num=1, clear=True)
assert (
True
), "wait_for_num_traces raises an exception after waiting for 1 trace." # wait_for_num_traces will throw an error if not received within 2 sec, so we expect to see an exception
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this assertion. If a ValueError is not raised on line 141 then this test will fail. If you want to add an assertion you can check the error message in the value error and ensure it's do to a timeout/missing spans and not some other failure.

Suggested change
assert (
True
), "wait_for_num_traces raises an exception after waiting for 1 trace." # wait_for_num_traces will throw an error if not received within 2 sec, so we expect to see an exception


# The DD_TRACE_AGENT_URL is validated using the tracer configuration. This approach avoids the need to modify the setup file to create additional containers at the specified URL, which would be unnecessarily complex.
@parametrize(
"library_env",
[{"DD_TRACE_AGENT_URL": "http://random-host:9999/", "DD_AGENT_HOST": "localhost", "DD_AGENT_PORT": "8126"}],
)
def test_dd_trace_agent_http_url_nonexistent(self, library_env, test_agent, test_library):
with test_library as t:
resp = t.get_tracer_config()
assert resp["dd_trace_agent_url"] == "http://random-host:9999/"
with pytest.raises(ValueError):
test_agent.wait_for_num_traces(num=1, clear=True)
assert (
True
), "wait_for_num_traces raises an exception after waiting for 1 trace." # wait_for_num_traces will throw an error if not received within 2 sec, so we expect to see an exception
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

3 changes: 2 additions & 1 deletion utils/build/docker/nodejs/parametric/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ app.get('/trace/config', (req, res) => {
'dd_trace_sample_ignore_parent': 'null', // not implemented in node
'dd_trace_otel_enabled': 'null', // not exposed in config object in node
'dd_env': config?.tags?.env !== undefined ? `${config.tags.env}` : 'null',
'dd_version': config?.tags?.version !== undefined ? `${config.tags.version}` : 'null'
'dd_version': config?.tags?.version !== undefined ? `${config.tags.version}` : 'null',
'dd_trace_agent_url': config?.url !== undefined ? `${config.url.href}` : 'null',
}
});
});
Expand Down
2 changes: 2 additions & 0 deletions utils/parametric/_library_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ def get_tracer_config(self) -> Dict[str, Optional[str]]:
"dd_trace_sample_ignore_parent": config_dict.get("dd_trace_sample_ignore_parent", None),
"dd_env": config_dict.get("dd_env", None),
"dd_version": config_dict.get("dd_version", None),
"dd_trace_agent_url": config_dict.get("dd_trace_agent_url", None),
}


Expand Down Expand Up @@ -729,6 +730,7 @@ def get_tracer_config(self) -> Dict[str, Optional[str]]:
"dd_trace_sample_ignore_parent": config_dict.get("dd_trace_sample_ignore_parent", None),
"dd_env": config_dict.get("dd_env", None),
"dd_version": config_dict.get("dd_version", None),
"dd_trace_agent_url": config_dict.get("dd_trace_agent_url", None),
mabdinur marked this conversation as resolved.
Show resolved Hide resolved
}


Expand Down
Loading