Skip to content

Commit 428de53

Browse files
waleedlatif1waleedlatif1
committed
fix(dynatrace): stop three silent failures found in a final read-through
All three turn a failed call into something that looks like a successful empty one, which is the worst shape for an observability integration — you cannot tell "nothing is wrong" from "the call did not work". `readJsonBody` swallowed any unparseable body and returned `{}`. A gateway HTML page, a captive-portal interstitial, or a truncated payload therefore mapped every field to null and read as "no problems found". Only genuinely empty bodies are tolerated now (201 from add-comment, 204 from log ingest); anything else that will not parse raises with a truncated preview. `ingest_logs` sent `[]` when the payload was missing or empty. Dynatrace answers 204 to that, so the tool reported `accepted: true` for a call that shipped no logs. It now fails loudly instead. `encodeDynatracePathSegment` percent-encoded the whole metric key and then regex-unescaped `%3A` back to `:`. Same output, but it undoes the encoder's work and hides the intent. Colons are structural in a metric key, so it now splits on them, encodes each part, and rejoins — which says that directly. Each fix has a test, and each test was confirmed to fail in isolation with only its own fix reverted.
1 parent 934563b commit 428de53

3 files changed

Lines changed: 57 additions & 6 deletions

File tree

‎apps/sim/tools/dynatrace/dynatrace.test.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,19 @@ describe('json request params', () => {
135135
expect(sent.eventTimeout).toBeUndefined()
136136
})
137137

138+
it('rejects an empty log payload instead of sending a no-op that reports success', () => {
139+
// Dynatrace answers 204 to `[]`, which would otherwise surface as accepted:true.
140+
expect(() => body(ingestLogsTool, { ...base, logs: [] })).toThrow(/at least one log event/)
141+
expect(() => body(ingestLogsTool, { ...base, logs: '' })).toThrow(/at least one log event/)
142+
expect(() => body(ingestLogsTool, { ...base, logs: undefined })).toThrow(
143+
/at least one log event/
144+
)
145+
})
146+
147+
it('rejects a malformed JSON string rather than silently dropping the payload', () => {
148+
expect(() => body(ingestLogsTool, { ...base, logs: '{not json' })).toThrow(/valid JSON/)
149+
})
150+
138151
it('omits optional event fields that were not provided', () => {
139152
const sent = body(ingestEventTool, {
140153
...base,
@@ -412,6 +425,22 @@ describe('response mapping', () => {
412425
expect(closed.comment?.content).toBe('fixed')
413426
})
414427

428+
it('raises on a non-JSON body instead of reporting an empty result', async () => {
429+
// A gateway HTML page or a truncated payload must not read as "no results".
430+
const html = new Response('<html><body>502 Bad Gateway</body></html>', {
431+
status: 200,
432+
headers: { 'content-type': 'text/html' },
433+
})
434+
await expect(listProblemsTool.transformResponse!(html)).rejects.toThrow(/non-JSON body/)
435+
})
436+
437+
it('still tolerates the genuinely empty bodies of 201 and 204', async () => {
438+
const created = await listProblemCommentsTool.transformResponse!(
439+
new Response(null, { status: 204 })
440+
)
441+
expect(created.output.comments).toEqual([])
442+
})
443+
415444
it('surfaces a 200 partial-success log ingestion body', async () => {
416445
const response = new Response(JSON.stringify({ error: { message: 'some invalid' } }), {
417446
status: 200,

‎apps/sim/tools/dynatrace/ingest_logs.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,15 @@ export const ingestLogsTool: ToolConfig<DynatraceIngestLogsParams, DynatraceInge
4646
url: (params) => buildDynatraceUrl(params.environmentUrl, '/logs/ingest'),
4747
method: 'POST',
4848
headers: (params) => dynatraceHeaders(params.apiToken),
49-
body: (params) => JSON.stringify(parseJsonParam(params.logs) ?? []),
49+
body: (params) => {
50+
const logs = parseJsonParam(params.logs)
51+
// Dynatrace answers 204 for an empty array, which would report a send that
52+
// never happened as a success. Fail loudly instead.
53+
if (logs === undefined || (Array.isArray(logs) && logs.length === 0)) {
54+
throw new Error('logs must contain at least one log event')
55+
}
56+
return JSON.stringify(logs)
57+
},
5058
},
5159

5260
/** Ingestion answers 204 with no body, or 200 with a partial-success body. */

‎apps/sim/tools/dynatrace/utils.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,18 @@ export function buildDynatraceUrl(
4646
}
4747

4848
/**
49-
* Encodes a path segment, leaving the `:` separators intact that Dynatrace metric
50-
* keys and transformation operators use (`builtin:host.cpu.usage:avg`).
49+
* Encodes a metric key for a URL path. In Dynatrace a `:` is structural — it
50+
* separates the metric key from its transformation operators
51+
* (`builtin:host.cpu.usage:avg`) — so each colon-delimited part is encoded and
52+
* the separators are rejoined verbatim, matching the unencoded form the API
53+
* reference uses in its own examples.
5154
*/
5255
export function encodeDynatracePathSegment(value: string): string {
53-
return encodeURIComponent(value.trim()).replace(/%3A/gi, ':')
56+
return value
57+
.trim()
58+
.split(':')
59+
.map((part) => encodeURIComponent(part))
60+
.join(':')
5461
}
5562

5663
/** Encodes an identifier for use in a URL path, tolerating copy-pasted whitespace. */
@@ -90,15 +97,22 @@ export function dynatraceHeaders(
9097

9198
/**
9299
* Reads a JSON response body, tolerating the empty bodies Dynatrace returns for
93-
* 201/204 responses.
100+
* 201 (comment created) and 204 (logs accepted).
101+
*
102+
* A non-empty body that will not parse is an error, not an empty result — a
103+
* gateway HTML page or a truncated payload would otherwise map to `{}` and
104+
* surface as "no results", which is indistinguishable from a genuinely empty
105+
* environment.
94106
*/
95107
export async function readJsonBody(response: Response): Promise<Record<string, unknown>> {
96108
const text = await response.text()
97109
if (!text.trim()) return {}
98110
try {
99111
return JSON.parse(text) as Record<string, unknown>
100112
} catch {
101-
return {}
113+
throw new Error(
114+
`Dynatrace returned a non-JSON body (HTTP ${response.status}): ${truncate(text.trim(), 200)}`
115+
)
102116
}
103117
}
104118

0 commit comments

Comments
 (0)