Skip to content

Commit 1aef337

Browse files
cursoragentMikeShi42
cursoragent
and
MikeShi42
committed
fix: simplify duration detection to exact match only
Remove regex-based division pattern matching. The duration expression can be arbitrary SQL, so substring or regex matching is fragile and error-prone. Now only applies duration formatting when the select valueExpression exactly equals the source's durationExpression. Co-authored-by: Mike Shi <mike@hyperdx.io>
1 parent 491207f commit 1aef337

File tree

2 files changed

+37
-141
lines changed

2 files changed

+37
-141
lines changed

‎packages/app/src/__tests__/source.test.ts‎

Lines changed: 32 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -59,165 +59,98 @@ describe('getTraceDurationNumberFormat', () => {
5959
expect(result).toBeUndefined();
6060
});
6161

62-
// --- exact raw expression match ---
62+
// --- exact match ---
6363

64-
it('detects exact raw duration expression with avg aggFn', () => {
65-
const result = getTraceDurationNumberFormat(TRACE_SOURCE, [
66-
{ valueExpression: 'Duration', aggFn: 'avg' },
67-
]);
68-
expect(result).toEqual({ output: 'duration', factor: 1e-9 });
69-
});
70-
71-
it('does not match raw fallback when expression only contains duration name', () => {
72-
expect(
73-
getTraceDurationNumberFormat(TRACE_SOURCE, [
74-
{ valueExpression: 'avg(Duration)' },
75-
]),
76-
).toBeUndefined();
77-
});
78-
79-
// --- ms expression variants ---
80-
81-
it('detects ms expression with parens: (Duration)/1e6', () => {
82-
expect(
83-
getTraceDurationNumberFormat(TRACE_SOURCE, [
84-
{ valueExpression: '(Duration)/1e6' },
85-
]),
86-
).toEqual({ output: 'duration', factor: 0.001 });
87-
});
88-
89-
it('detects ms expression without parens: Duration/1e6', () => {
64+
it('matches when valueExpression exactly equals durationExpression', () => {
9065
expect(
9166
getTraceDurationNumberFormat(TRACE_SOURCE, [
92-
{ valueExpression: 'Duration/1e6' },
67+
{ valueExpression: 'Duration', aggFn: 'avg' },
9368
]),
94-
).toEqual({ output: 'duration', factor: 0.001 });
69+
).toEqual({ output: 'duration', factor: 1e-9 });
9570
});
9671

97-
it('detects ms expression with spaces: Duration / 1e6', () => {
72+
it('matches without aggFn (raw expression passed through)', () => {
9873
expect(
9974
getTraceDurationNumberFormat(TRACE_SOURCE, [
100-
{ valueExpression: 'Duration / 1e6' },
75+
{ valueExpression: 'Duration' },
10176
]),
102-
).toEqual({ output: 'duration', factor: 0.001 });
77+
).toEqual({ output: 'duration', factor: 1e-9 });
10378
});
10479

105-
it('detects ms expression with parens and spaces: (Duration) / 1e6', () => {
106-
expect(
107-
getTraceDurationNumberFormat(TRACE_SOURCE, [
108-
{ valueExpression: '(Duration) / 1e6' },
109-
]),
110-
).toEqual({ output: 'duration', factor: 0.001 });
111-
});
80+
// --- non-matching expressions ---
11281

113-
it('detects ms expression with inner spaces: ( Duration )/1e6', () => {
82+
it('does not match expressions that only contain the duration name', () => {
11483
expect(
11584
getTraceDurationNumberFormat(TRACE_SOURCE, [
116-
{ valueExpression: '( Duration )/1e6' },
85+
{ valueExpression: 'avg(Duration)' },
11786
]),
118-
).toEqual({ output: 'duration', factor: 0.001 });
87+
).toBeUndefined();
11988
});
12089

121-
// --- seconds expression variants ---
122-
123-
it('detects seconds expression with parens: (Duration)/1e9', () => {
90+
it('does not match division expressions', () => {
12491
expect(
12592
getTraceDurationNumberFormat(TRACE_SOURCE, [
126-
{ valueExpression: '(Duration)/1e9' },
93+
{ valueExpression: 'Duration/1e6' },
12794
]),
128-
).toEqual({ output: 'duration', factor: 1 });
129-
});
130-
131-
it('detects seconds expression without parens: Duration/1e9', () => {
95+
).toBeUndefined();
13296
expect(
13397
getTraceDurationNumberFormat(TRACE_SOURCE, [
134-
{ valueExpression: 'Duration/1e9' },
98+
{ valueExpression: '(Duration)/1e6' },
13599
]),
136-
).toEqual({ output: 'duration', factor: 1 });
137-
});
138-
139-
it('detects seconds expression with spaces: Duration / 1e9', () => {
100+
).toBeUndefined();
140101
expect(
141102
getTraceDurationNumberFormat(TRACE_SOURCE, [
142103
{ valueExpression: 'Duration / 1e9' },
143104
]),
144-
).toEqual({ output: 'duration', factor: 1 });
145-
});
146-
147-
it('detects seconds expression with parens and spaces: ( Duration ) / 1e9', () => {
148-
expect(
149-
getTraceDurationNumberFormat(TRACE_SOURCE, [
150-
{ valueExpression: '( Duration ) / 1e9' },
151-
]),
152-
).toEqual({ output: 'duration', factor: 1 });
105+
).toBeUndefined();
153106
});
154107

155-
// --- non-matching expressions ---
156-
157-
it('does not match unrelated expressions containing the duration name', () => {
108+
it('does not match modified or similar-named expressions', () => {
158109
expect(
159110
getTraceDurationNumberFormat(TRACE_SOURCE, [
160111
{ valueExpression: 'Duration * 2' },
161112
]),
162113
).toBeUndefined();
163114
expect(
164115
getTraceDurationNumberFormat(TRACE_SOURCE, [
165-
{ valueExpression: 'round(Duration / 1e6, 2)' },
116+
{ valueExpression: 'LongerDuration' },
166117
]),
167118
).toBeUndefined();
168119
expect(
169120
getTraceDurationNumberFormat(TRACE_SOURCE, [
170-
{ valueExpression: 'LongerDuration' },
121+
{ valueExpression: 'round(Duration / 1e6, 2)' },
171122
]),
172123
).toBeUndefined();
173124
});
174125

175126
// --- aggFn filtering ---
176127

177-
it('returns undefined for count aggFn on duration', () => {
128+
it('returns undefined for count aggFn', () => {
178129
expect(
179130
getTraceDurationNumberFormat(TRACE_SOURCE, [
180131
{ valueExpression: 'Duration', aggFn: 'count' },
181132
]),
182133
).toBeUndefined();
183134
});
184135

185-
it('returns undefined for count_distinct aggFn on duration', () => {
136+
it('returns undefined for count_distinct aggFn', () => {
186137
expect(
187138
getTraceDurationNumberFormat(TRACE_SOURCE, [
188139
{ valueExpression: 'Duration', aggFn: 'count_distinct' },
189140
]),
190141
).toBeUndefined();
191142
});
192143

193-
it('detects duration with sum aggFn', () => {
194-
expect(
195-
getTraceDurationNumberFormat(TRACE_SOURCE, [
196-
{ valueExpression: 'Duration', aggFn: 'sum' },
197-
]),
198-
).toEqual({ output: 'duration', factor: 1e-9 });
199-
});
200-
201-
it('detects duration with quantile aggFn', () => {
202-
expect(
203-
getTraceDurationNumberFormat(TRACE_SOURCE, [
204-
{ valueExpression: 'Duration', aggFn: 'quantile' },
205-
]),
206-
).toEqual({ output: 'duration', factor: 1e-9 });
207-
});
208-
209-
it('detects duration with min/max aggFn', () => {
210-
expect(
211-
getTraceDurationNumberFormat(TRACE_SOURCE, [
212-
{ valueExpression: 'Duration', aggFn: 'min' },
213-
]),
214-
).toEqual({ output: 'duration', factor: 1e-9 });
215-
expect(
216-
getTraceDurationNumberFormat(TRACE_SOURCE, [
217-
{ valueExpression: 'Duration', aggFn: 'max' },
218-
]),
219-
).toEqual({ output: 'duration', factor: 1e-9 });
220-
});
144+
it.each(['sum', 'min', 'max', 'quantile', 'avg', 'any', 'last_value'])(
145+
'detects duration with %s aggFn',
146+
aggFn => {
147+
expect(
148+
getTraceDurationNumberFormat(TRACE_SOURCE, [
149+
{ valueExpression: 'Duration', aggFn },
150+
]),
151+
).toEqual({ output: 'duration', factor: 1e-9 });
152+
},
153+
);
221154

222155
it('detects duration with combinator aggFn like avgIf', () => {
223156
expect(

‎packages/app/src/source.ts‎

Lines changed: 5 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -423,31 +423,17 @@ function isDurationPreservingAggFn(aggFn: string | undefined): boolean {
423423
return DURATION_PRESERVING_AGG_FNS.has(baseFn);
424424
}
425425

426-
function escapeRegExp(s: string): string {
427-
return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
428-
}
429-
430-
/**
431-
* Builds a regex that matches `<durationExpr> / 1e<N>` with optional
432-
* surrounding parentheses and whitespace. Handles variants like:
433-
* (Duration)/1e6, Duration/1e6, Duration / 1e6, (Duration) / 1e6
434-
*/
435-
function buildDivisionPattern(durationExpr: string, exponent: number): RegExp {
436-
const escaped = escapeRegExp(durationExpr);
437-
return new RegExp(`^\\(?\\s*${escaped}\\s*\\)?\\s*/\\s*1e${exponent}$`);
438-
}
439-
440426
/**
441427
* Returns a NumberFormat for duration display if the chart config's select
442-
* expressions reference a trace source's durationExpression. Returns undefined
443-
* if no match is detected.
428+
* expressions exactly match a trace source's durationExpression. Returns
429+
* undefined if no match is detected.
444430
*
445431
* Only applies when the aggregate function preserves the unit of the input
446432
* (e.g. avg, min, max, sum, p95). Functions like count and count_distinct
447433
* produce dimensionless values and are skipped.
448434
*
449-
* The returned format uses the `duration` output with a `factor` that converts
450-
* from the raw value's precision to seconds.
435+
* Uses exact match only — the duration expression can be arbitrary SQL,
436+
* so substring or regex matching would be fragile.
451437
*/
452438
export function getTraceDurationNumberFormat(
453439
source: TSource | undefined,
@@ -461,35 +447,12 @@ export function getTraceDurationNumberFormat(
461447

462448
const durationExpr = source.durationExpression;
463449
const precision = source.durationPrecision ?? 9;
464-
const msExponent = precision - 3;
465-
const secondsExponent = precision;
466-
467-
const msPattern = buildDivisionPattern(durationExpr, msExponent);
468-
const secondsPattern = buildDivisionPattern(durationExpr, secondsExponent);
469450

470451
for (const sel of selectExpressions) {
471452
if (!sel.valueExpression) continue;
472453
if (!isDurationPreservingAggFn(sel.aggFn)) continue;
473454

474-
const expr = sel.valueExpression;
475-
476-
if (msPattern.test(expr)) {
477-
return {
478-
output: 'duration',
479-
factor: 0.001,
480-
};
481-
}
482-
483-
if (secondsPattern.test(expr)) {
484-
return {
485-
output: 'duration',
486-
factor: 1,
487-
};
488-
}
489-
490-
// Exact match only for the raw duration expression to avoid
491-
// over-matching expressions like "Duration/1e6" as raw precision.
492-
if (expr === durationExpr) {
455+
if (sel.valueExpression === durationExpr) {
493456
return {
494457
output: 'duration',
495458
factor: Math.pow(10, -precision),

0 commit comments

Comments
 (0)