-
Notifications
You must be signed in to change notification settings - Fork 14
Add tests for startup log behavior in tracer libraries #6241
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
base: main
Are you sure you want to change the base?
Conversation
|
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 98790a0 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
4a29519 to
4fd5c1a
Compare
b90773c to
19694ec
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19694ec8ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else: | ||
| try: | ||
| # Node.js and Ruby log to stdout, other libraries only log stderr | ||
| if context.library in ("nodejs", "ruby"): |
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.
Optional: Ideally a test should not have different execution paths for different languages. You should make multiple tests and use the manifests to activate the relevant one for each language. This let's the easy win process catch any evolution of the tracer behavior in the furture.
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'm going to keep the test as-is for now. I plan to update the other languages to fix them in the very short term via DataDog/dd-trace-rb#5342 and DataDog/dd-trace-js#7470 - one is merged and the other one is close to being merged
Once these are in, I can remove all of the language-conditional logic, with the exception of the .NET log file logic. However, I believe it is helpful to have in the same test to showcase the deviation of .NET, rather than appearing as two valid behaviors
|
I noticed that you put the AI generated label on this PR, would you mind telling me how much of the code was generated, with what tool and how much prompting you did (one prompt or multiple, and whether you needed to give the llm a lot of information or not). I'm asking to evaluate the effectiveness of the AI rules in the repo |
I started using Cursor, but moved to Claude Code afterwards since it tended to perform better in this repo. I'm a bit of a novice at them, so I'm just experimenting Nearly all of the code was generated, but it took a ton of prompting to identify the different language-specific behaviors present and it couldn't figure out C++/PHP at all. After making the changes, it was a bit verbose/hard to follow so I prompted it to clean itself up One piece that was a struggle was the runtime for format.sh and re-running parametric tests as it tried to fix the tests - this was the clear largest time spend in this task. As a result, I asked Claude to optimize both of them, resulting in #6256 and #6258, which I have as draft since I haven't fully wrapped my head around each line of code. If you're more comfortable in those parts of the codebase, I'd encourage you to check them out since it may take me some time to go through them and understand the potential consequences of the optimizations |
Moved test skip declarations from inline decorators (@missing_feature, @incomplete_test_app) to manifest files for cpp, nodejs, php, and rust. Also added span trigger in test_startup_logs_diagnostic_agent_unreachable to ensure tracers attempt agent connection. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Sorry meant to update DataDog/dd-trace-rb#5342 instead of this PR. |
genesor
left a comment
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.
Looks ok overall, the readability could be improve to have most of the test follow a single path where possible.
| test_library.dd_flush() | ||
|
|
||
| # For .NET, startup logs are written to a file instead of stdout/stderr | ||
| if context.library == "dotnet": |
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.
Could we get this condition reworked (and the similar ones in the other tests) to an simpler if / else if / else
I don't think it's useful to have dotnet in its own if and other language in another if in the else condition.
| if match: | ||
| logger.error(logs) | ||
| pytest.fail( | ||
| # f"Startup log found when DD_TRACE_STARTUP_LOGS=false. " |
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.
Shouldn't this be uncommented so we understand what's going on ?
| if logs is not None: | ||
| startup_log_pattern = r"DATADOG (TRACER )?CONFIGURATION( - (CORE|TRACING|PROFILING|.*))?" | ||
| if re.search(startup_log_pattern, logs, re.IGNORECASE): | ||
| pytest.fail( | ||
| "Startup log found in .NET log file when DD_TRACE_STARTUP_LOGS=false. " | ||
| f"Content (first 1000 chars): {logs[:1000]}" | ||
| ) |
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 could be factorized with the same check below
nccatoni
left a comment
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.
Thank you for the changes and for the insides in your AI development workflow. Everything looks good to me now but you should also get an approval from someone familiar with the feature
Addresses code review feedback by consolidating duplicated log retrieval logic and simplifying conditional structures. Changes include: - Extract STARTUP_LOG_PATTERN to module-level constant - Add unified _get_startup_logs() helper function - Replace nested if/else with clearer if/elif/else pattern - Improve error messages to include matched pattern information This reduces code duplication (~60 lines to ~30 shared lines) and makes the test logic easier to follow and maintain. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Thanks for the review! I've addressed all the feedback: ✅ Line 50 (nested if/else): Replaced with Changes:
|
Motivation
Add comprehensive tests for tracer startup log behavior across supported languages. These tests verify that tracers correctly emit startup configuration logs with the appropriate default behavior and when explicitly configured via
DD_TRACE_STARTUP_LOGS, ensuring proper observability and configuration visibility.Changes
Adds
tests/parametric/test_startup_logs.pywith four test cases:test_startup_logs_default: Verifies default startup log behavior whenDD_TRACE_STARTUP_LOGSis not settest_startup_logs_enabled: Verifies that startup logs are emitted whenDD_TRACE_STARTUP_LOGS=truetest_startup_logs_disabled: Verifies that startup logs are suppressed whenDD_TRACE_STARTUP_LOGS=falsetest_startup_logs_diagnostic_agent_unreachable: Verifies diagnostic messages appear when the agent is unreachableLanguage-Specific Handling
The tests handle language-specific differences:
dotnet-tracer-managed*) instead of stdout/stderrRecent Updates
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present