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

Chore: Import only lodash submodules #2041

Merged
merged 6 commits into from
Dec 12, 2023
Merged

Chore: Import only lodash submodules #2041

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b06811
add ld submd
anshgoyalevil Dec 10, 2023
eb4546d
fix lodash syntax
anshgoyalevil Dec 11, 2023
25f22c5
Merge branch 'main' into ldsh
anshgoyalevil Dec 11, 2023
cf5fe52
fix lodash syntax
anshgoyalevil Dec 11, 2023
b6ea0db
use js syntax
anshgoyalevil Dec 11, 2023
8cc2692
use built-in map, remove concat, revert var name change
anshgoyalevil Dec 12, 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
22 changes: 12 additions & 10 deletions packages/jaeger-ui/src/components/TracePage/TraceStatistics/generateDropdownValue.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import _ from 'lodash';
import _map from 'lodash/map';
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

+1

import _flatten from 'lodash/flatten';
import _uniq from 'lodash/uniq';
import _filter from 'lodash/filter';
import _concat from 'lodash/concat';
import { Trace } from '../../../types/trace';
import { ITableSpan } from './types';

Expand Down Expand Up @@ -41,15 +45,13 @@ function getValueTagIsPicked(tableValue: ITableSpan[], trace: Trace, nameSelecto
}
availableTags = [...new Set(availableTags)];

const tags = _(availableTags).map('tags').flatten().value();
let tagKeys = _(tags).map('key').uniq().value();
tagKeys = _.filter(tagKeys, function calc(o) {
return o !== nameSelectorTitle;
});
const tags = _map(availableTags, 'tags');
let spansWithFilterTag = _uniq(_map(_flatten(tags), 'key'));
spansWithFilterTag = _filter(spansWithFilterTag, o => o !== nameSelectorTitle);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Thanks 🚀

availableTags = [];
availableTags.push(serviceName);
availableTags.push(operationName);
availableTags = availableTags.concat(tagKeys);
availableTags = availableTags.concat(spansWithFilterTag);

return availableTags;
}
Expand Down Expand Up @@ -77,9 +79,9 @@ function getValueNoTagIsPicked(trace: Trace, nameSelectorTitle: string) {

export function generateDropdownValue(trace: Trace) {
const allSpans = trace.spans;
const tags = _(allSpans).map('tags').flatten().value();
const tagKeys = _(tags).map('key').uniq().value();
const values = _.concat(serviceName, operationName, tagKeys);
const tags = _flatten(_map(allSpans, 'tags'));
const tagKeys = _uniq(_map(tags, 'key'));
const values = _concat(serviceName, operationName, tagKeys);
return values;
}

Expand Down
21 changes: 10 additions & 11 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 @@ -13,7 +13,8 @@
// limitations under the License.

import memoizeOne from 'memoize-one';
import * as _ from 'lodash';
import _uniqBy from 'lodash/uniqBy';
import _map from 'lodash/map';
import DRange from 'drange';
import { Trace, Span } from '../../../types/trace';
import { ITableSpan } from './types';
Expand Down Expand Up @@ -115,20 +116,18 @@ function valueFirstDropdown(selectedTagKey: string, trace: Trace) {
const allSpans = trace.spans;
// all possibilities that can be displayed
if (selectedTagKey === serviceName) {
const temp = _.chain(allSpans)
.groupBy(x => x.process.serviceName)
.map((value, key) => ({ key }))
.uniq()
.value();
const temp = _map(
_uniqBy(allSpans, x => x.process.serviceName),
x => ({ key: x.process.serviceName })
);
Copy link
Member

Choose a reason for hiding this comment

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

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()

?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@yurishkuro yurishkuro Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@yurishkuro yurishkuro Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

and perhaps you may remove the following loop

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

Copy link
Member

Choose a reason for hiding this comment

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

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));

Copy link
Member Author

Choose a reason for hiding this comment

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

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

for (let i = 0; i < temp.length; i++) {
allDiffColumnValues.push(temp[i].key);
}
} else if (selectedTagKey === operationName) {
const temp = _.chain(allSpans)
.groupBy(x => x.operationName)
.map((value, key) => ({ key }))
.uniq()
.value();
const temp = _map(
_uniqBy(allSpans, x => x.operationName),
x => ({ key: x.operationName })
);
for (let i = 0; i < temp.length; i++) {
allDiffColumnValues.push(temp[i].key);
}
Expand Down
Loading