feat(flamegraph): added sub-second units support for trace visualization #1418

StasDachinsky · 2022-08-17T17:56:40Z

Brief

Support sub-second units for trace visualization #1393

Changes

added ms and μs support for trace_samples formatter
added tests for μs, ms, hour, day, month and year

Concerns

CLAassistant · 2022-08-17T17:56:56Z

All committers have signed the CLA.

codecov · 2022-08-17T17:59:52Z

Codecov Report

Merging #1418 (0ff3738) into main (b1170fa) will increase coverage by 0.07%.
The diff coverage is 68.75%.

❗ Current head 0ff3738 differs from pull request most recent head 5362965. Consider uploading reports for the commit 5362965 to get more accurate results

@@            Coverage Diff             @@
##             main    #1418      +/-   ##
==========================================
+ Coverage   68.49%   68.56%   +0.07%     
==========================================
  Files         129      129              
  Lines        4246     4255       +9     
  Branches     1136     1139       +3     
==========================================
+ Hits         2908     2917       +9     
  Misses       1333     1333              
  Partials        5        5

Impacted Files	Coverage Δ
packages/pyroscope-flamegraph/src/format/format.ts	`72.61% <68.75%> (+1.80%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-08-17T18:00:04Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
webapp/public/assets/app.js	416.77 KB (+0.02% 🔺)	8.4 s (+0.02% 🔺)	3.2 s (+1.04% 🔺)	11.5 s
webapp/public/assets/app.css	15.23 KB (0%)	305 ms (0%)	0 ms (+100% 🔺)	305 ms
webapp/public/assets/styles.css	9.46 KB (0%)	190 ms (0%)	0 ms (+100% 🔺)	190 ms
packages/pyroscope-flamegraph/dist/index.js	90.87 KB (+0.1% 🔺)	1.9 s (+0.1% 🔺)	1.8 s (+29.02% 🔺)	3.6 s
packages/pyroscope-flamegraph/dist/index.node.js	90.72 KB (+0.1% 🔺)	1.9 s (+0.1% 🔺)	743 ms (+71.23% 🔺)	2.6 s
packages/pyroscope-flamegraph/dist/index.css	7.02 KB (0%)	141 ms (0%)	0 ms (+100% 🔺)	141 ms

eh-am · 2022-08-17T18:29:34Z

packages/pyroscope-flamegraph/src/format/format.spec.ts

        expect(df.format(100, 100)).toBe('1.00 second');
        expect(df.format(2000, 100)).toBe('20.00 seconds');
-        expect(df.format(2012.3, 100)).toBe('20.12 seconds');
+        expect(df.format(2012.3, 100)).toBe('20123.00 ms');


I don't quite get this, why do we use seconds for 2000, but ms for 2012.3?

eh-am · 2022-08-17T18:33:59Z

packages/pyroscope-flamegraph/src/format/format.ts

@@ -1,5 +1,6 @@
 /* eslint-disable max-classes-per-file */
 import { Units } from '@pyroscope/models/src';
+import _last from 'lodash/last';


Do we really need this dependency?

Agreed, I'm also not a big fan of these — there's value in keeping the list of dependencies as low as possible.

Agreed, this will be removed

eh-am · 2022-08-17T19:29:28Z

packages/pyroscope-flamegraph/src/format/format.ts

+    [60, 'minute'],
+    [60, 'hour'],
+    [24, 'day'],
+    [30, 'month'],
+    [12, 'year'],


for completion it's worth adding tests for these

petethepig · 2022-08-18T08:38:12Z

packages/pyroscope-flamegraph/src/format/format.spec.ts

        expect(df.format(8000, 100)).toBe('80.00 seconds');
+        expect(df.format(374.12, 100)).toBe('3741200.00 μs');


this one also doesn't seem right

petethepig · 2022-08-18T08:52:01Z

packages/pyroscope-flamegraph/src/format/format.ts

+
+  units = '';
+
+  constructor(maxDur: number, units?: string) {


units isn't really used by any of the users of SubSecondDurationFormatter, right? Therefore I'd remove it altogether.

petethepig · 2022-08-18T09:08:03Z

packages/pyroscope-flamegraph/src/format/format.ts

+  format(samples: number, sampleRate: number): string {
+    let n = samples / sampleRate / this.divider;
+
+    if (n && !Number.isInteger(n) && this.divider === 1) {


This logic is hard to understand, and it is faulty (see test cases above). I would not invest any more time into it.

I think you could use existing DurationFormatter, but modify it in two ways:

Add milliseconds and microseconds to durations array, with 1000 multipliers each.

Modify line 138 to be let n = samples / sampleRate * 1000000 / this.divider to compensate for extra precision.

And the rest of it should work just fine.

@StasDachinsky let me know what you think.

And by the way, if that's the case (and these are the only changes we're making to this formatter) we probably don't need a separate class and instead we could have a constructor argument in the existing DurationFormatter.

/cc @Rperry2174 — let us know if there's more changes that need to happen. E.g jaeger does double numbers

Ok, I will update the code according to your recommendations

@petethepig Can you please explain what you mean by the constructor argument for DurationFormatter? Should I keep the current logic when the suffix is selected by the constructor argument maxDur, or do I need to update it to allow DurationFormatter to select the suffix based on the value passed in the format function (like in the jaeger), but only for trase_samples?

@StasDachinsky by that I meant that we'd add a new argument to DurationFormatter constructor, something like enableSubsecondPrecision, and then if you want to include sub-second precision you'd call it with that extra boolean, e.g:

new DurationFormatter(123, '', true)

This way we can make it so that in all other cases DurationFormatter is still working the same way it used to (with no sub-second precision) and it only does work with sub-second precision in tracing mode.

petethepig

It all looks good, I think the last thing we need to do is make it so that sub second duration only show up for trace_samples. That will also fix the cypress test.

StasDachinsky requested review from eh-am and Rperry2174 August 17, 2022 17:56

eh-am reviewed Aug 17, 2022

View reviewed changes

Rperry2174 requested a review from petethepig August 17, 2022 18:55

eh-am reviewed Aug 17, 2022

View reviewed changes

petethepig reviewed Aug 18, 2022

View reviewed changes

eh-am changed the title ~~feat: added sub-second units support for trace visualization~~ feat(flamegraph): added sub-second units support for trace visualization Aug 18, 2022

petethepig requested changes Aug 24, 2022

View reviewed changes

StasDachinsky added 2 commits August 24, 2022 14:03

feat: added sub-second units support for trace visualization

7e8a088

fix: DurationFormatter logic updated, more tests added

361b3e9

StasDachinsky force-pushed the feat/sub-second-units-support branch from 7f62082 to 0ff3738 Compare August 24, 2022 12:23

fix(flamegraph): added enableSubsecondPrecision prop

5362965

StasDachinsky force-pushed the feat/sub-second-units-support branch from 0ff3738 to 5362965 Compare August 24, 2022 12:24

Ivanikitin requested a review from petethepig August 26, 2022 17:37

petethepig approved these changes Aug 30, 2022

View reviewed changes

petethepig merged commit 21f6550 into main Aug 30, 2022

petethepig deleted the feat/sub-second-units-support branch August 30, 2022 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(flamegraph): added sub-second units support for trace visualization #1418

feat(flamegraph): added sub-second units support for trace visualization #1418

StasDachinsky commented Aug 17, 2022 •

edited

Loading

CLAassistant commented Aug 17, 2022 •

edited

Loading

codecov bot commented Aug 17, 2022 •

edited

Loading

github-actions bot commented Aug 17, 2022 •

edited

Loading

eh-am Aug 17, 2022

eh-am Aug 17, 2022

petethepig Aug 18, 2022

StasDachinsky Aug 18, 2022

eh-am Aug 17, 2022

petethepig Aug 18, 2022

petethepig Aug 18, 2022 •

edited

Loading

petethepig Aug 18, 2022

petethepig Aug 18, 2022

StasDachinsky Aug 18, 2022

StasDachinsky Aug 18, 2022 •

edited

Loading

petethepig Aug 22, 2022

petethepig left a comment

		expect(df.format(8000, 100)).toBe('80.00 seconds');
		expect(df.format(374.12, 100)).toBe('3741200.00 μs');

feat(flamegraph): added sub-second units support for trace visualization #1418

feat(flamegraph): added sub-second units support for trace visualization #1418

Conversation

StasDachinsky commented Aug 17, 2022 • edited Loading

Brief

Changes

Concerns

CLAassistant commented Aug 17, 2022 • edited Loading

codecov bot commented Aug 17, 2022 • edited Loading

Codecov Report

github-actions bot commented Aug 17, 2022 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petethepig Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StasDachinsky Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petethepig left a comment

Choose a reason for hiding this comment

StasDachinsky commented Aug 17, 2022 •

edited

Loading

CLAassistant commented Aug 17, 2022 •

edited

Loading

codecov bot commented Aug 17, 2022 •

edited

Loading

github-actions bot commented Aug 17, 2022 •

edited

Loading

petethepig Aug 18, 2022 •

edited

Loading

StasDachinsky Aug 18, 2022 •

edited

Loading