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.

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

Khanayan123/implement dd trace agent url system test #3073

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

Conversation

khanayan123
Copy link
Contributor

Motivation

config consistency initiative

Changes

implement dd trace agent url system test

@khanayan123 khanayan123 force-pushed the khanayan123/implement_dd_trace_agent_url_system_test branch from a1802d9 to afedfdf Compare September 19, 2024 15:06
@khanayan123 khanayan123 marked this pull request as ready for review September 19, 2024 15:45
@khanayan123 khanayan123 marked this pull request as draft September 19, 2024 16:06
@khanayan123 khanayan123 marked this pull request as ready for review September 19, 2024 16:41
Copy link
Contributor

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

LGTM with some really minor nitpicks

Copy link
Contributor

@link04 link04 left a comment

Choose a reason for hiding this comment

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

Need to update all manifests to Test_Config_TraceAgentURL instead of Test_Config_DDTraceAgentURL, after that LGTM(passes for .NET).

@@ -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

Comment on lines 143 to 145
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

Comment on lines 158 to 160
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

utils/parametric/_library_client.py Show resolved Hide resolved
Copy link
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

A small typo to fix in all manifests

@@ -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
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

@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants