feat: add Dynatrace integration - #282
Conversation
Add supported Environment API operations and Grail-backed distributed trace queries. Allow the Claude triage app token to request workflows:write for explicitly scoped CI changes. Closes #281 Generated-with: OpenAI Codex (GPT-5)
|
Claude finished @kolatts's task in 5m 2s —— View job PR Review: feat: add Dynatrace integration
Approved. Clean, complete integration that follows established patterns. What's solid:
Minor findings (inline comments posted):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| for (let attempt = 0; response.state === 'RUNNING' || response.state === 'NOT_STARTED'; attempt++) { | ||
| if (!response.requestToken) throw new PncliError('Dynatrace did not return a query request token'); | ||
| if (attempt >= 11) throw new PncliError('Dynatrace trace query did not finish within 60 seconds'); |
There was a problem hiding this comment.
Suggestion — poll-loop timing claim is approximately right, but fragile if long-polling isn't honoured
The attempt >= 11 guard allows 11 poll calls (attempts 0–10). Each poll sends request-timeout=5000, which instructs Dynatrace to hold the connection for up to 5 s before responding. So worst-case wall time is ≈ 55 s (11 × 5 s) plus the initial execute call — close enough to the "60 seconds" claim.
The concern: if Dynatrace returns a RUNNING response immediately (e.g. the long-poll param isn't honoured or the server is under load), all 11 attempts burn in milliseconds, and the user sees the timeout error after < 1 s, which is confusing. Consider adding a small floor sleep when the response comes back faster than expected, or using a wall-clock deadline instead of an attempt counter:
const deadline = Date.now() + 60_000;
while (response.state === 'RUNNING' || response.state === 'NOT_STARTED') {
if (!response.requestToken) throw new PncliError('Dynatrace did not return a query request token');
if (Date.now() >= deadline) throw new PncliError('Dynatrace trace query did not finish within 60 seconds');
// ...poll...
}This is a suggestion, not a blocker — the current approach works correctly when Dynatrace honours the long-poll timeout.
| results.openshift = { ok: null, message: 'not configured' }; | ||
| } | ||
|
|
||
| if (cfg.dynatrace.baseUrl && cfg.dynatrace.apiToken) { |
There was a problem hiding this comment.
Nit — config test doesn't exercise the platform token
The connectivity test fires a dynatrace (classic Environment API) call, but there's no check for platformUrl / platformToken. If the platform credentials are misconfigured the user won't know until they hit a runtime error in dynatrace trace. The same gap exists in config check (line ~530).
It's fine for a first pass — adding dynatrace_platform as a separate result key when platformUrl && platformToken are set would close the gap. Not blocking.
| claude_args: | | ||
| --model sonnet --max-turns 300 --allowedTools "Read,Glob,Grep,Edit,Write,Bash(gh issue list:*),Bash(gh issue comment:*),Bash(gh issue close:*),Bash(gh issue edit:*),Bash(gh label create:*),Bash(gh pr view:*),Bash(npm run build:*),Bash(npm run typecheck:*),Bash(npm run lint:*),Bash(npm test:*),Bash(git checkout -b:*),Bash(git add:*),Bash(git commit:*),Bash(git push:*)" | ||
| additional_permissions: | | ||
| workflows: write |
There was a problem hiding this comment.
Security note — workflows: write is a meaningful permission escalation for an automated agent processing untrusted input
The triage agent now has the ability to modify .github/workflows/ files. The prompt already includes strong anti-injection guardrails (explicit untrusted-input framing, allow-list for when workflow edits are permitted, security assessment gate), so the risk is mitigated in practice.
Worth keeping in mind: this is the permission most valuable to an adversary trying to exploit a prompt-injection attack via a crafted issue body. If you see an issue that requests a workflow change and whose body contains anything suspicious, double-check the resulting commit carefully before merging. The existing "modify workflow or CI files only when the issue explicitly requests..." guardrail in the prompt is the main control here.
There was a problem hiding this comment.
LGTM — Approve
Clean, complete integration that follows the established patterns throughout.
What's solid
Architecture: All seven required files from CLAUDE.md updated together — types, config resolution, http client, config commands, CLI registration, skill doc, skill index. Nothing missing.
Security: DQL injection in the trace query is correctly neutralised. The regex /^[0-9a-f]{16,32}$/i validates the trace ID to hex-only before it's interpolated into the DQL string, so there's no injection surface. Credential masking covers both apiToken and platformToken.
Auth: Correct token scheme for each endpoint — Api-Token for the classic Environment API, Bearer for the Grail platform API. Tests verify both paths.
Pagination: The allPages / do-while loop matches Dynatrace's cursor-based pagination. totalCount returned is the actual fetched count rather than the API's pre-pagination estimate, which is the right call since we always exhaust all pages.
Config resolution: Env vars have highest precedence for all four Dynatrace fields, consistent with the project's global precedence rule.
CLI UX: The wizard splits classic vs. platform config cleanly, verifies the classic connection before saving, and gives correct setup instructions. config check and config test treat Dynatrace consistently with other services.
Minor findings (see inline comments)
- Poll loop (
commands.ts:90): attempt-counter works correctly with Dynatrace long-polling, but a wall-clock deadline would be more robust if the server returnsRUNNINGwithout honouringrequest-timeout. Suggestion only. - Platform creds not verified (
config/commands.ts:268):config test/config checkonly exercise the classic API; misconfigured platform token goes undetected untildynatrace traceis called. Easy follow-up, not a blocker. workflows: writepermission (claude-triage.yml:167): noted the tradeoff — mitigated by strong prompt guardrails, but worth awareness.
|
Claude Code is working…
|
Use a wall-clock trace polling deadline and validate optional Grail platform credentials during config checks. Generated-with: OpenAI Codex (GPT-5)
|
Claude finished @kolatts's task in 6m 25s —— View job PR Review: feat: add Dynatrace integration
Approved. Both substantive findings from the previous review pass have been addressed in this commit. What changed since the first reviewPoll-loop timing — the wall-clock deadline suggestion is now implemented. Platform credential coverage — New finding (nit, inline comments posted)
Rest of the integration is solid
|
| // Human-readable table to stdout | ||
| const services = ['jira', 'bitbucket', 'github', 'confluence', 'sonar', 'sde', 'ado', 'jenkins', 'udeploy', 'artifactory', 'checkmarx', 'servicenow', 'contrast', 'sonatypeiq', 'openshift'] as const; | ||
| const services = ['jira', 'bitbucket', 'github', 'confluence', 'sonar', 'sde', 'ado', 'jenkins', 'udeploy', 'artifactory', 'checkmarx', 'servicenow', 'contrast', 'sonatypeiq', 'openshift', 'dynatrace', 'dynatrace_platform'] as const; | ||
| const labelWidth = 14; |
There was a problem hiding this comment.
Nit — labelWidth = 14 is too narrow for dynatrace_platform
dynatrace_platform is 18 characters. svc.padEnd(14) on a string already longer than 14 is a no-op, so the status column for that row will be shifted 4 characters to the right compared to every other service, misaligning the table.
| const labelWidth = 14; | |
| const labelWidth = 20; |
Same change needed on line 615 (the --pretty branch).
| if (opts.pretty) { | ||
| const services = ['jira', 'bitbucket', 'github', 'confluence', 'sonar', 'sde', 'ado', 'jenkins', 'udeploy', 'artifactory', 'checkmarx', 'servicenow', 'contrast', 'sonatypeiq', 'openshift'] as const; | ||
| const services = ['jira', 'bitbucket', 'github', 'confluence', 'sonar', 'sde', 'ado', 'jenkins', 'udeploy', 'artifactory', 'checkmarx', 'servicenow', 'contrast', 'sonatypeiq', 'openshift', 'dynatrace', 'dynatrace_platform'] as const; | ||
| const labelWidth = 14; |
There was a problem hiding this comment.
Same labelWidth issue as line 597 — needs to be 20 here too to align dynatrace_platform.
| const labelWidth = 14; | |
| const labelWidth = 20; |
There was a problem hiding this comment.
LGTM — Approve
Clean follow-up commit that addresses both substantive findings from the first review pass.
What changed since the last review
Poll-loop robustness (previously: suggestion) — now implemented correctly. commands.ts uses a Date.now() + 60_000 wall-clock deadline instead of an attempt counter, and includes a 1-second floor sleep when the server returns a RUNNING response faster than expected. Both the deadline guard and the fast-response floor were added; the logic is correct.
Platform credential coverage (previously: nit) — both config test and config check now exercise the platform token separately, reporting results as dynatrace_platform. The half-configured case (one of the two platform fields set, not both) is correctly flagged as an error in both commands.
New finding (nit — inline comments posted)
labelWidth = 14 in the config check table renderer (commands.ts:597 and :615) is too narrow for dynatrace_platform (18 chars). padEnd(14) is a no-op on strings already longer than 14, so that row's status column shifts 4 characters right. Bump to 20 to keep the table aligned.
Rest of the integration holds up
- All seven required CLAUDE.md files updated
- Auth scheme correct:
Api-Tokenfor classic Environment API,Bearerfor Grail platform - Hex-only validation of trace ID before DQL interpolation — no injection surface
encodeURIComponenton entity/problem IDs in URL path segments- Env vars have highest precedence for all four Dynatrace fields
- Both tokens masked in
maskConfig - Tests cover both auth paths and env-var resolution
1 participant
Summary
workflows: writefor explicitly scoped workflow/CI issuesWhy
The original automated implementation for #281 was committed only in an ephemeral Actions checkout and could not be pushed. Its proposed trace handling also relied on
traces.lookup, which checks trace presence for cross-environment tracing but does not retrieve trace spans. This implementation rebuilds the integration against the supported Dynatrace Environment and Platform APIs.User impact
Users can configure a classic Dynatrace API token for entities/problems and, optionally, a platform token for Grail-backed trace queries. All commands return pncli's structured JSON output and require no external Dynatrace CLI.
Validation
npm run typechecknpm run lintnpm test(396 tests)npm run buildCloses #281