Add css obfuscation test [APMSP-2764]#6648
Conversation
|
|
4871555 to
0cf62ff
Compare
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 45e38cc | Docs | Datadog PR Page | Give us feedback! |
0cf62ff to
714e99a
Compare
| "DD_TRACE_TRACER_METRICS_ENABLED": "true", # java | ||
| }, | ||
| agent_env={ | ||
| "DD_APM_SQL_OBFUSCATION_MODE": "normalize_only", |
There was a problem hiding this comment.
I don't see anything about disabling the obfuscation in the spec is it done by setting the obfuscation version to 0 (if not maybe we need a scenario for the missing/version 0 case)
There was a problem hiding this comment.
It's not a setting from the CSS spec but to avoid breaking changes in the obfuscation behavior when switching from agent obfuscation to client obfuscation I decided that following as much of the config as possible would be preferable.
https://github.com/DataDog/datadog-agent/blob/main/pkg/config/config_template.yaml#L1633
There was a problem hiding this comment.
Sorry I wasn't really clear what I meant was:
- Agent obfuscation config doesn't seem to be covered in the current spec, maybe it should be updated (CC @ichinaski)
- Does this scenario implies
Agent version 0 or missingif not maybe we want to run a scenario for this case.
There was a problem hiding this comment.
- Yes, I think this is a shortcoming of the spec currently, because with CSS disabled the agent would have followed the obfuscation config.
- No it's exactly like
TRACE_STATS_COMPUTATIONbut with config such as obfuscation makes the 4 spans different so that they end up in different buckets so it's visible. I'll add a similar test with a missing version 👍🏽
VianneyRuhlmann
left a comment
There was a problem hiding this comment.
LGTM, the idea for a version 0 scenario can be addressed in a follow-up PR if needed
1c3b465 to
38567bc
Compare
9d3f272 to
1fcdc3a
Compare
…ternal queries
SQLite internally executes SELECT QUOTE(?) which naturally contains '?'
as a parameter placeholder. Narrowing the resource filter from
startswith('SELECT') to startswith('SELECT * FROM users') excludes
these internal queries from the no-obfuscation assertions.
…ries
Django ORM generates queries like 'SELECT ? FROM app_customuser...' that
don't match the test's expected resource pattern. Filter to
startswith('SELECT * FROM users') to only capture queries from the
/rasp/sqli endpoint used in setup.
| def test_obfuscation(self): | ||
| stats_count = 0 | ||
| hits = 0 | ||
| top_hits = 0 | ||
| resource = "SELECT * FROM users WHERE id = ?" | ||
| # wait for 10 seconds to be sure all the buckets are flushed (better than be flaky) | ||
| for s in interfaces.agent.get_stats(resource): | ||
| stats_count += 1 | ||
| for s in interfaces.agent.get_stats(): | ||
| if s["Type"] != "sql" or "?" not in s["Resource"]: | ||
| continue | ||
| logger.debug(f"asserting on {s}") | ||
| hits += s["Hits"] | ||
| top_hits += s["TopLevelHits"] | ||
| assert s["Type"] == "sql", "expect 'sql' type" | ||
| assert stats_count <= 4, ( | ||
| "expect <= 4 stats" | ||
| ) # Normally this is exactly 2 but in certain high load this can flake and result in additional payloads where hits are split across two payloads | ||
| assert hits == top_hits >= 4, "expect at least 4 'OK' hits and top level hits across all payloads" | ||
|
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45e38cceca
ℹ️ 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".
|
|
||
|
|
||
| @features.client_side_stats_supported | ||
| @scenarios.trace_stats_computation_future_obfuscation_version |
There was a problem hiding this comment.
Regenerate the scenario map for new stats tests
For future PRs that touch only tests/stats/test_stats.py, CI selects impacted end-to-end scenarios from tests/test_the_test/scenarios.json via utils/scripts/compute_libraries_and_scenarios.py; I checked this commit's map with rg "Obfuscation|OBFUSCATION" and none of the new Test_Client_Stats_*Obfuscation* nodeids are present. As a result, these new TRACE_STATS_COMPUTATION_* scenarios are not added to inputs.scenarios, so the workflow steps added for them will not run on test-file-only changes. Please regenerate/update the scenario map with the new nodeids.
Useful? React with 👍 / 👎.
| if: steps.build.outcome == 'success' && !cancelled() && contains(inputs.scenarios, '"TRACE_STATS_COMPUTATION"') | ||
| run: ./run.sh TRACE_STATS_COMPUTATION | ||
| - name: Run TRACE_STATS_COMPUTATION_OBFUSCATION_DISABLED scenario | ||
| if: always() && steps.build.outcome == 'success' && contains(inputs.scenarios, '"TRACE_STATS_COMPUTATION_OBFUSCATION_DISABLED"') |
There was a problem hiding this comment.
Preserve failure gating for obfuscation scenario steps
In this workflow section, the existing scenario steps use the default successful-previous-steps gating plus !cancelled(), but these new steps add always() and omit !cancelled(). GitHub Actions treats always() as a status override, so when an earlier scenario in this job fails or a run is cancelled, these obfuscation scenarios can still start instead of skipping like the surrounding scenario steps, wasting CI time and making cancellations less responsive. Use the same steps.build.outcome == 'success' && !cancelled() && ... pattern unless these are intentionally post-failure cleanup steps.
Useful? React with 👍 / 👎.
btw I don't get the fail in test_the_tests because the new scenariofixedTRACE_STATS_COMPUTATION_OBFUSCATION_DISABLEDhas agent_env variables that the existingTRACE_STATS_COMPUTATIONdoesn't so they're not equivalent.TODO
add a test to verify peer tag obfuscation/aggregation client side:(later in another PR) feat: use ip quantization when aggregating peer tags for trace stats libdatadog#1944Motivation
/statsChanges
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