Skip to content
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

Speed up Trace Statistics view calculation #1941

Merged
merged 9 commits into from
Nov 29, 2023
Merged

Speed up Trace Statistics view calculation #1941

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
046da80
Speed up Trace Statistics view calculation
maxgaponov Nov 3, 2023
15b77b1
Refactored the changes
maxgaponov Nov 17, 2023
0a19fee
Merge branch 'main' into speedup-trace-statistics-calculation
maxgaponov Nov 17, 2023
6f7cf46
Refactored the changes
maxgaponov Nov 20, 2023
2f2d0b9
Refactor the changes
maxgaponov Nov 28, 2023
8d3593d
Refactor the changes
maxgaponov Nov 29, 2023
ba8fde9
Refactor the changes
maxgaponov Nov 29, 2023
0074d0f
undo no-op changes
yurishkuro Nov 29, 2023
d2943f3
Merge branch 'main' into speedup-trace-statistics-calculation
yurishkuro Nov 29, 2023
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
47 changes: 35 additions & 12 deletions packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import memoizeOne from 'memoize-one';
import * as _ from 'lodash';
import DRange from 'drange';
import { Trace, Span } from '../../../types/trace';
Expand All @@ -21,8 +22,37 @@ import colorGenerator from '../../../utils/color-generator';
const serviceName = 'Service Name';
const operationName = 'Operation Name';

function parentChildOfMap(allSpans: Span[]): Record<string, Span[]> {
const parentChildOfMap: Record<string, Span[]> = {};
allSpans.forEach(s => {
if (s.references) {
// Filter for CHILD_OF we don't want to calculate FOLLOWS_FROM (prod-cons)
const parentIDs = s.references.filter(r => r.refType === 'CHILD_OF').map(r => r.spanID);
parentIDs.forEach((pID: string) => {
parentChildOfMap[pID] = parentChildOfMap[pID] || [];
parentChildOfMap[pID].push(s);
});
}
});
return parentChildOfMap;
}

const memoizedParentChildOfMap = memoizeOne(parentChildOfMap);

function getChildOfSpans(parentID: string, allSpans: Span[]): Span[] {
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
return memoizedParentChildOfMap(allSpans)[parentID] || [];
}

function getChildOfDrange(parentID: string, allSpans: Span[]) {
const childrenDrange = new DRange();
getChildOfSpans(parentID, allSpans).forEach(s => {
// -1 otherwise it will take for each child a micro (incluse,exclusive)
childrenDrange.add(s.startTime, s.startTime + (s.duration <= 0 ? 0 : s.duration - 1));
});
return childrenDrange;
}

function computeSelfTime(span: Span, allSpans: Span[]): number {
if (!span.hasChildren) return span.duration;
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
// We want to represent spans as half-open intervals like [startTime, startTime + duration).
// This way the subtraction preserves the right boundaries. However, DRange treats all
// intervals as inclusive. For example,
Expand All @@ -32,18 +62,11 @@ function computeSelfTime(span: Span, allSpans: Span[]): number {
// We should've ended up with length 9-4=5, but we got 3.
// To work around that, we multiply start/end times by 10 and subtract one from the end.
// So instead of [1-10] we get [10-99]. This makes the intervals work like half-open.
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
const spanRange = new DRange(10 * span.startTime, 10 * (span.startTime + span.duration) - 1);
// Only keep CHILD_OF spans. FOLLOWS_FROM spans do not block the parent.
const children = allSpans.filter(
each =>
span.childSpanIds.includes(each.spanID) &&
each.references[0].spanID === span.spanID &&
each.references[0].refType === 'CHILD_OF'
if (!span.hasChildren) return span.duration;
const spanRange = new DRange(span.startTime, span.startTime + span.duration - 1).subtract(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can change the logic away from *10. DRange does not work correctly on the raw timestamps (see the comments above).

Side note: we need a test that illustrates that.

Copy link
Member

@yurishkuro yurishkuro Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand why the calculations need to change - the speed-up is coming from building a map once instead of n^2 looping, so why not change just that part and leave the calculations intact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the change we use DRange(begin, end - 1) to represent halp-open intervals. The length of this DRange is (end-1)-begin+1=end-begin because DRange treats all intervals as inclusive.

In the example above (1, 10) will be represented as DRange(1, 9); (4, 8) will be represented as DRange(4, 7). So first.substract(second) will be range([1..3], [8..9]) (length = 3+2=5).

As I understand there is a mistake in the comment in the calculation of Drange.length.

Copy link
Member

Choose a reason for hiding this comment

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

That still doesn't answer my question why calculations need to change, it a PR that is about efficiency of the code.

If you want to dig down to the exact reasons for *10 logic, you can see the comment as well as the discussion on the original PR that introduced it. TLDR is that DRange does not understand open/close intervals, and subtraction works incorrectly for our purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. It shouldn't be changed in this PR. Returned to the initial version.

getChildOfDrange(span.spanID, allSpans)
);
children.forEach(child => {
spanRange.subtract(10 * child.startTime, 10 * (child.startTime + child.duration) - 1);
});
return Math.round(spanRange.length / 10);
return spanRange.length;
}

function computeColumnValues(trace: Trace, span: Span, allSpans: Span[], resultValue: any) {
Expand Down
Loading