Skip to content

fix: escape periods in attribute keys during flatten/unflatten #3800

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.

Sign up for GitHub

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

Closed
deepshekhardas wants to merge 1 commit into from
Closed

fix: escape periods in attribute keys during flatten/unflatten #3800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/core/src/v3/utils/flattenAttributes.ts
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Map keys with dots are not escaped (pre-existing inconsistency)

At line 121, Map string keys are interpolated directly into the prefix without dot escaping: this.#processValue(value, \${prefix || "map"}.${keyStr}`, depth). This is inconsistent with the new escaping for regular object keys at line 204 (key.replace(/./g, DOT_ESCAPE)). If a Map has a key like "a.b"`, the dot won't be escaped, leading to incorrect unflattening. However, this is a pre-existing issue — Map key handling was already inconsistent before this PR, and Maps are a relatively niche case for OTEL attributes. Not filing as a bug since it's pre-existing, but worth noting for completeness.

(Refers to lines 120-121)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Attributes } from "@opentelemetry/api";

export const NULL_SENTINEL = "$@null((";
export const CIRCULAR_REFERENCE_SENTINEL = "$@circular((";
const DOT_ESCAPE = "$@dot";
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Escape sequence $@dot starts with $, causing keys beginning with . to be silently dropped by removePrivateProperties

The $@dot escape sequence starts with $, which conflicts with the removePrivateProperties filter at apps/webapp/app/v3/eventRepository/common.server.ts:155 that drops all keys starting with $. When a user has an object key starting with . (e.g., {".foo": "bar"}), flattenAttributes escapes it to "$@dotfoo". This key is stored in the database. When later read back, removePrivateProperties sees the key starts with $ and filters it out before unflattenAttributes ever runs. The property is silently lost. Before this PR, a key like ".foo" would be stored as ".foo" and would not be filtered (it doesn't start with $). While keys starting with . are uncommon, this is a silent data loss regression.

Affected data flow
  1. User logs {".hidden": "value"} from a task
  2. SDK flattens to {"$@dothidden": "value"}
  3. Server stores $@dothidden as attribute key
  4. On read, removePrivateProperties filters it out (starts with $)
  5. Property is silently lost from span display
Prompt for agents 
The escape sequence $@dot starts with $, which collides with the convention in removePrivateProperties (apps/webapp/app/v3/eventRepository/common.server.ts:155) that filters out all attribute keys starting with $. This means any user object key starting with a literal dot (.) will be escaped to a key starting with $@dot, which then gets silently dropped.

The simplest fix is to choose an escape sequence that does NOT start with $ or ctx. — the two prefixes filtered by removePrivateProperties. For example, using __dot__ or ~dot~ or any sequence that doesn't start with a character used for internal attribute prefixes.

Files to update:
- packages/core/src/v3/utils/flattenAttributes.ts: change the DOT_ESCAPE constant (line 5)
- packages/core/test/flattenAttributes.test.ts: update test expectations to match new escape sequence
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


const DEFAULT_MAX_DEPTH = 128;

Expand Down Expand Up @@ -200,7 +201,8 @@ class AttributeFlattener {
break;
}

const newPrefix = `${prefix ? `${prefix}.` : ""}${Array.isArray(obj) ? `[${key}]` : key}`;
const escapedKey = Array.isArray(obj) ? `[${key}]` : key.replace(/\./g, DOT_ESCAPE);
const newPrefix = `${prefix ? `${prefix}.` : ""}${escapedKey}`;
Comment on lines +204 to +205
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Round-trip data corruption for keys naturally containing the $@dot escape sequence

The escape sequence $@dot is never itself escaped during flattening, so keys that naturally contain the literal string $@dot are silently corrupted during round-trip. During flattenAttributes, key.replace(/\./g, DOT_ESCAPE) won't modify a key like "a$@dotb" (no dots to replace), so it stays as "a$@dotb". But during unflattenAttributes at flattenAttributes.ts:285, part.split(DOT_ESCAPE).join(".") transforms "a$@dotb" into "a.b", corrupting the key. For example: {"a$@dotb": 1} → flatten → {"a$@dotb": 1} → unflatten → {"a.b": 1}. This is a fundamental flaw in the escape mechanism — proper escaping requires that the escape sequence itself be escaped (e.g., $@dot → $@dot$@dot or a different sentinel).

Prompt for agents 
The escape mechanism is not reversible because the escape sequence itself is not escaped. When flattenAttributes encounters a key like "a$@dotb" (no dots), it passes it through unchanged. But unflattenAttributes unconditionally replaces all occurrences of $@dot with dots, corrupting the key to "a.b".

To fix this properly, you need a two-phase escape:
1. First escape any existing occurrences of the escape marker (e.g., replace "$@dot" with "$@dot$@dot" or use a dedicated escape-of-escape like "$@esc"), THEN replace dots with the escape marker.
2. In unflattenAttributes, reverse the process: first unescape the dot marker, then unescape the escape-of-escape marker.

Alternatively, choose an escape sequence that is guaranteed not to collide (e.g., a longer/more unique sentinel), though this only reduces probability rather than eliminating the issue.

Files to update:
- packages/core/src/v3/utils/flattenAttributes.ts: both the escaping in doFlatten (line 204) and unescaping in unflattenAttributes (line 285)
- packages/core/test/flattenAttributes.test.ts: add a test for keys containing the escape sequence
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


if (Array.isArray(value)) {
for (let i = 0; i < value.length; i++) {
Expand Down Expand Up @@ -280,17 +282,18 @@ export function unflattenAttributes(

const parts = key.split(".").reduce(
(acc, part) => {
if (part.startsWith("[") && part.endsWith("]")) {
const unescaped = part.split(DOT_ESCAPE).join(".");
if (unescaped.startsWith("[") && unescaped.endsWith("]")) {
// Handle array indices more precisely
const match = part.match(/^\[(\d+)\]$/);
const match = unescaped.match(/^\[(\d+)\]$/);
if (match && match[1]) {
acc.push(parseInt(match[1]));
} else {
// Remove brackets for non-numeric array keys
acc.push(part.slice(1, -1));
acc.push(unescaped.slice(1, -1));
}
} else {
acc.push(part);
acc.push(unescaped);
}
return acc;
},
Expand Down
24 changes: 24 additions & 0 deletions packages/core/test/flattenAttributes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,30 @@
import { flattenAttributes, unflattenAttributes } from "../src/v3/utils/flattenAttributes.js";

describe("flattenAttributes", () => {
it("escapes periods in object keys", () => {
const obj = { "Key 0.002mm": 31.4 };
const flattened = flattenAttributes(obj);
expect(flattened).toEqual({ "Key 0$@dot002mm": 31.4 });
expect(unflattenAttributes(flattened)).toEqual(obj);
});

it("handles multiple periods in a single key", () => {
const obj = { "a.b.c": "value" };
const flattened = flattenAttributes(obj);
expect(flattened).toEqual({ "a$@dotb$@dotc": "value" });
expect(unflattenAttributes(flattened)).toEqual(obj);
});

it("handles period keys nested inside regular objects", () => {
const obj = { meta: { "Key 0.002mm": 31.4, "version": 2 } };
const flattened = flattenAttributes(obj);
expect(flattened).toEqual({
"meta.Key 0$@dot002mm": 31.4,
"meta.version": 2,
});
expect(unflattenAttributes(flattened)).toEqual(obj);
});

it("handles number keys correctly", () => {
expect(flattenAttributes({ bar: { "25": "foo" } })).toEqual({ "bar.25": "foo" });
expect(unflattenAttributes({ "bar.25": "foo" })).toEqual({ bar: { "25": "foo" } });
Expand Down