-
Notifications
You must be signed in to change notification settings - Fork 9
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
Config Consistency Tests for DD_TRACE_<INTEGRATION>_ENABLED #3060
Conversation
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.
left some nits but overall looks good to me
doc="", | ||
scenario_groups=[ScenarioGroup.ESSENTIALS], | ||
) | ||
|
||
tracing_config_nondefault_3 = EndToEndScenario( | ||
"TRACING_CONFIG_NONDEFAULT_3", weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false"}, doc="", | ||
"TRACING_CONFIG_NONDEFAULT_3", | ||
weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false", "DD_TRACE_Kafka_ENABLED": "true"}, |
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.
nit: casing
weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false", "DD_TRACE_Kafka_ENABLED": "true"}, | |
weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false", "DD_TRACE_KAFKA_ENABLED": "true"}, |
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.
This casing may actually be required for the .NET SDK to recognize the flag on Linux so we might need to keep this
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.
Indeed so the other languages need to add the env var as their tracer expects it as they enable the test(some languages may even have different integrations names).
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.
@zacharycmontoya Well, the casing is required for the PHP extension to recognize the flag on Linux, but in uppercase.
If I read it correctly at least Java and Ruby are using full uppercase too. From a quick glance at docs I couldn't determine what exactly Python and node are doing.
So... we have to change this in any case in at least one place.
I'd propose to universally move to fully uppercase. I find uppercase also more intuitive, in the midst of everything else being uppercase.
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.
I'd propose to universally move to fully uppercase. I find uppercase also more intuitive, in the midst of everything else being uppercase.
Agree, all our configuration env var are upper case. It looks weird to have one that mix both. By the why, is there a RFC for that ?
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.
So it was previously implemented like that, not sure I could find the RFC for that since is so old, but we have agreed that for these efforts of the Config Consistency RFC it should be added that we always expected a all Upper cased env var.
doc="", | ||
scenario_groups=[ScenarioGroup.ESSENTIALS], | ||
) | ||
|
||
tracing_config_nondefault_3 = EndToEndScenario( | ||
"TRACING_CONFIG_NONDEFAULT_3", weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false"}, doc="", | ||
"TRACING_CONFIG_NONDEFAULT_3", | ||
weblog_env={"DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING": "false", "DD_TRACE_Kafka_ENABLED": "true"}, |
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.
We should leave a comment stating that kafka is being used to validate DD_TRACE__ENABLED configurations. There's nothing special about kafka, it just an integration that is consistent across all libraries (🤞).
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.
It's not present in PHP though.
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.
Yeah the plan was to use a different existing endpoint and env var for PHP since is the only one left out.
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.
Testing : ok
CI: ok (failures are not related)
There is only the question of lowecase/uppercase. Not my decision here (even if I have a strong opinion 😉 ) So I let @bwoebi the charge to approve the PR.
Motivation
Our APMAPI-284 Jira card where we need to write a test that validates the outcome of using the setting with both
true
andfalse
values so that we can test if the auto instrumented library behaves as expected.Changes
tracing_config_nondefault
scenario and added the env var with the value offalse
and totracing_config_nondefault_2
with the value offtrue
.Workflow
codeowners
file quickly.🚀 Once your PR is reviewed, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>]
, double-check that only<language>
is impacted by the changebuild-XXX-image
label is present