Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/add consistent config system tests #3745
base: main
Are you sure you want to change the base?
Khanayan123/add consistent config system tests #3745
Changes from 21 commits
b0eea10
264a755
252ad46
c1d9118
f53f99c
60d2872
72fd00a
b792106
41dd15a
efa3ef3
e871399
d963739
a4d445f
6cf3b31
918fbbb
bb91710
78b9e7b
e954a94
6a2057e
bf111d5
dd4a9c6
3afd035
7f51151
f5b4d41
59c1a37
3429244
9f484c5
54b89be
2d42831
69a1e60
0eb65dd
17b45bd
6945a62
3d14959
9ed1223
4c75e27
7d68776
d2e0312
75e4d32
bda76a0
9dfa2a3
6e8f305
c363d24
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should this be a "bug"?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could just remove it from all the tests since I believe no language currently requires the glob environment variable
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.
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/
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make parsing easier
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.
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