Chore: Import only lodash submodules #2041

anshgoyalevil · 2023-12-10T15:02:09Z

Which problem is this PR solving?

resolves Import only lodash submodules #2032

Description of the changes

Import only lodash submodules

How was this change tested?

locally

Super cool that it reduced the size taken by lodash bundle by 70.59% (master branch vs this)

Before:

lodash: 773.55 KB

After:

lodash: 227.43 KB

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

codecov · 2023-12-10T15:05:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4bdb8ac) 96.55% compared to head (8cc2692) 96.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2041      +/-   ##
==========================================
- Coverage   96.55%   96.54%   -0.01%     
==========================================
  Files         255      255              
  Lines        7625     7611      -14     
  Branches     1983     1983              
==========================================
- Hits         7362     7348      -14     
  Misses        263      263

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2023-12-10T16:01:06Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

+        _groupBy(allSpans, x => x.process.serviceName),
+        (value, key) => ({ key })
+      )
+    );


If I understand this, the goal is to get a unique list of service names, right? If so, why use groupBy at all, wouldn't this work (in scala syntax):

spans.map(_.process.serviceName).uniq()

?

Yes. I have used a more concise function uniqBy here now.

better, but could this not be written in fluent style like _uniqBy(...).map()?

and perhaps you may remove the following loop

_uniqBy(allSpans, span => span.process.serviceName) .foreach(span => allDiffColumnValues.push(span.process.serviceName))

in fact, allDiffColumnValues is assigned once, so no need to push

allDiffColumnValues = _uniqBy(allSpans, span => span.process.serviceName) .map(span => span.process.serviceName);

I still think the most intuitive form is this:

allDiffColumnValues = _uniq(_map(span => span.process.serviceName));

wow, I literally ignored that for loop with a focus to replace the former lodash syntax with the new one.
This one seems neat

yurishkuro · 2023-12-10T16:06:16Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx

-  });
+  const tags = _map(availableTags, 'tags');
+  let tagKeys = _uniq(_map(_flatten(tags), 'key'));
+  tagKeys = _filter(tagKeys, o => o !== nameSelectorTitle);


This may be the equivalent, but could we do better? First, availableTags var is overloaded: starts a set of spans, then set of strings, bad design. And all these lodash ops are revisiting the same data that was already processed in the loop above - could we just calculate what we need right there?

At least we should rename the var to spansWithFilterTag

I tried playing with the former logic but it is getting a bit complex to accommodate the work we are taking out of lodash functions to the loops.

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro · 2023-12-11T16:21:01Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx

-  });
+  const tags = _map(availableTags, 'tags');
+  let spansWithFilterTag = _uniq(_map(_flatten(tags), 'key'));
+  spansWithFilterTag = _filter(spansWithFilterTag, o => o !== nameSelectorTitle);


this collection does not contain spans, now it's even more confusing. I was referring to L46 and above, where the objects in question are spans, but the collection is called availableTags

Got it. Thanks 🚀

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil · 2023-12-11T17:10:36Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx

@@ -12,19 +12,22 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-import _ from 'lodash';
+import _map from 'lodash/map';


We can get rid of this _map submodule too, using the built-in JS function. Should I go ahead with that change?

yurishkuro · 2023-12-11T18:23:01Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx

+  const tags = spansWithFilterTag.map(o => o.tags);
+  let tagKeys = _uniq(_map(_flatten(tags), 'key'));
+  tagKeys = tagKeys.filter(o => o !== nameSelectorTitle);
+  spansWithFilterTag = [];


these are not spans anymore, this is just output. Could this not be simplified like this?

return [serviceName, operationName, ... tagKeys];

or _concat()

Used the Spread syntax, to remove _concat completely.

yurishkuro · 2023-12-11T18:24:13Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx

  const allSpans = trace.spans;
  if (nameSelectorTitle === serviceName) {
-    availableTags.push(operationName);
+    spansWithFilterTag.push(operationName);


these are not spans

yurishkuro · 2023-12-11T18:26:26Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx

    }
  }
-  availableTags = [...new Set(availableTags)];
+  spansWithFilterTag = [...new Set(spansWithFilterTag)];


could also do x = _uniq(x) here, more explicit than set-to-array transform.

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro

🎉

add ld submd

9b06811

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil requested a review from a team as a code owner December 10, 2023 15:02

anshgoyalevil requested review from yurishkuro and removed request for a team December 10, 2023 15:02

yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Dec 10, 2023

yurishkuro reviewed Dec 10, 2023

View reviewed changes

anshgoyalevil added 3 commits December 11, 2023 08:43

fix lodash syntax

eb4546d

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

Merge branch 'main' into ldsh

25f22c5

fix lodash syntax

cf5fe52

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro reviewed Dec 11, 2023

View reviewed changes

use js syntax

b6ea0db

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil commented Dec 11, 2023

View reviewed changes

yurishkuro reviewed Dec 11, 2023

View reviewed changes

use built-in map, remove concat, revert var name change

8cc2692

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro approved these changes Dec 12, 2023

View reviewed changes

yurishkuro merged commit 46f89af into jaegertracing:main Dec 12, 2023
10 checks passed

yurishkuro mentioned this pull request Dec 12, 2023

Imported only lodash submodules #2033

Closed

anshgoyalevil deleted the ldsh branch May 13, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Import only lodash submodules #2041

Chore: Import only lodash submodules #2041

anshgoyalevil commented Dec 10, 2023 •

edited

Loading

codecov bot commented Dec 10, 2023 •

edited

Loading

yurishkuro Dec 10, 2023

anshgoyalevil Dec 11, 2023

yurishkuro Dec 11, 2023 •

edited

Loading

yurishkuro Dec 11, 2023 •

edited

Loading

yurishkuro Dec 11, 2023

anshgoyalevil Dec 11, 2023

yurishkuro Dec 10, 2023

yurishkuro Dec 10, 2023

anshgoyalevil Dec 11, 2023

yurishkuro Dec 11, 2023

anshgoyalevil Dec 11, 2023

anshgoyalevil Dec 11, 2023

yurishkuro Dec 11, 2023

yurishkuro Dec 11, 2023

yurishkuro Dec 11, 2023

anshgoyalevil Dec 12, 2023

yurishkuro Dec 11, 2023

yurishkuro Dec 11, 2023

yurishkuro left a comment

Chore: Import only lodash submodules #2041

Chore: Import only lodash submodules #2041

Conversation

anshgoyalevil commented Dec 10, 2023 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Dec 10, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

yurishkuro Dec 11, 2023 • 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

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

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

yurishkuro left a comment

Choose a reason for hiding this comment

anshgoyalevil commented Dec 10, 2023 •

edited

Loading

codecov bot commented Dec 10, 2023 •

edited

Loading

yurishkuro Dec 11, 2023 •

edited

Loading

yurishkuro Dec 11, 2023 •

edited

Loading