Top level apis #499

jake-bassett · 2021-01-13T22:41:43Z

Description

We want to remove the nested Service -> APIs list and just use a flat list. This PR makes that change.

It also:

Adds the ability to configure dropdown filters to the table controls
Introduces a secondary-entity-specification so we can have a second display: Entity column in the table that links to the service

…evel-apis

projects/common/src/utilities/types/types.ts

codecov · 2021-01-13T22:45:18Z

Codecov Report

Merging #499 (16d27dc) into main (4002603) will decrease coverage by 0.14%.
The diff coverage is 54.28%.

@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
- Coverage   85.69%   85.54%   -0.15%     
==========================================
  Files         751      754       +3     
  Lines       15417    15479      +62     
  Branches     1834     1836       +2     
==========================================
+ Hits        13212    13242      +30     
- Misses       2174     2206      +32     
  Partials       31       31

Impacted Files	Coverage Δ
projects/components/src/select/select.component.ts	`93.75% <ø> (ø)`
...specifiers/mapped-attribute-specification.model.ts	`57.14% <ø> (ø)`
...src/pages/apis/services/services-list.dashboard.ts	`100.00% <ø> (ø)`
src/app/shared/navigation/navigation.component.ts	`100.00% <ø> (ø)`
...d/widgets/table/table-widget-renderer.component.ts	`58.82% <34.61%> (-8.53%)`	⬇️
...raphql/entity/entities-values-data-source.model.ts	`46.15% <46.15%> (ø)`
...nts/src/table/controls/table-controls.component.ts	`96.07% <50.00%> (-3.93%)`	⬇️
.../widgets/table/table-widget-select-filter.model.ts	`60.00% <60.00%> (ø)`
.../attribute/entities-attribute-data-source.model.ts	`75.00% <75.00%> (ø)`
projects/components/src/public-api.ts	`100.00% <100.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4002603...16d27dc. Read the comment docs.

anandtiwary · 2021-01-13T22:51:19Z

projects/components/src/table/controls/table-controls.component.ts

  public searchPlaceholder?: string = 'Search...';

+  @Input()
+  public selectMap?: Map<string, SelectFilter>;


What should be the key and the value? The property name could be more descriptive.

I am cleaning up this code. Agreed its too confusing. Update incoming.

anandtiwary · 2021-01-13T22:52:48Z

projects/components/src/table/controls/table-controls.component.ts


+        <!-- Selects -->
+        <ht-select
+          *ngFor="let selectFilter of this.selectMap | keyvalue"


*ngFor="let selectFilter of this.selectMap | keyvalue"

can we call this selectFilterEntry as the current name conflicts with the value type.

Also, why is this a map? What does key represent here?

So if I understand, each map entry represents a separate select dropdown? Do we need support for multiple (considering we don't for any other control type)?

Correct. In the case of multiple select boxes (we currently have a use case for three of them on the same table), the key is how the user knows which filter changes. It is passed back to the user.

projects/components/src/table/controls/table-controls.component.ts

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts

...s/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-select-filter.model.ts

...ability/src/shared/dashboard/data/graphql/specifiers/secondary-entity-specification.model.ts

...ared/graphql/request/builders/specification/entity/secondary-entity-specification-builder.ts

aaron-steinfeld · 2021-01-14T14:46:31Z

projects/components/src/table/controls/table-controls.component.ts


+        <!-- Selects -->
+        <ht-select
+          *ngFor="let selectFilter of this.selectMap | keyvalue"


So if I understand, each map entry represents a separate select dropdown? Do we need support for multiple (considering we don't for any other control type)?

aaron-steinfeld · 2021-01-14T14:52:00Z

...s/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-select-filter.model.ts

+
+  public getData(): Observable<PrimitiveValue[]> {
+    return this.api
+      .getData<Dictionary<PrimitiveValue>[]>()


What is this model supposed to represent? where is it getting data from?

This is getting the data to populate the dropdown filter. We query for the Entities, then pull out the values by attribute. frontend and backend as shown in the screenshots.

This is the code that is related to your questions here about using SERVICE instead of API. It shows why we can't index into a SERVICE entity returned by api.getData() when we are working with API entities and attributes in this model.

I proposed one tweak in another comment, but alternately, do we even need a fetch? Could we use the fetched data to populate this and just pass the data to the filter model to transform into a filters? That's basically what we're doing with the existing table filters right? The downside would be the same, that our filter options are only based on the queried data.

Aaaaah, right. Agree they should be consistent. Fixed mine to just provide an array of attributes instead of the full Entity. Much better.

As for the single fetch, I think that the independent fetch is still a better route. I don't really like the way I did the header filters where we tie it to the table data. It couple everything too closely. Granted, if you are filtering on a table, the data between the filer and the table should match so technically they are coupled.

But by having them independently fetched we have more freedom to fetch results that might not be fetched by the table due to pagination or other filters (maybe debatable if that second part is good or bad). It also allows us to have filters for columns we don't even fetch/show.

I think I'd like to go with what we have now (along with your other suggestions).

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts

projects/observability/src/pages/apis/services/services-list.dashboard.ts

...ability/src/shared/dashboard/data/graphql/specifiers/secondary-entity-specification.model.ts

projects/components/src/table/controls/table-controls.component.ts

...s/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-select-filter.model.ts

...y/src/shared/dashboard/data/graphql/entity/attribute/entities-attribute-data-source-model.ts

aaron-steinfeld · 2021-01-15T20:01:27Z

...s/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-select-filter.model.ts

+
+  public getData(): Observable<PrimitiveValue[]> {
+    return this.api
+      .getData<Dictionary<PrimitiveValue>[]>()


I proposed one tweak in another comment, but alternately, do we even need a fetch? Could we use the fetched data to populate this and just pass the data to the filter model to transform into a filters? That's basically what we're doing with the existing table filters right? The downside would be the same, that our filter options are only based on the queried data.

...y/src/shared/dashboard/data/graphql/entity/attribute/entities-attribute-data-source-model.ts

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts

…evel-apis

projects/observability/src/pages/apis/services/services-list.dashboard.ts

.../observability/src/shared/dashboard/data/graphql/entity/entities-values-data-source.model.ts

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts

skjindal93 · 2021-01-18T12:09:36Z

projects/components/src/table/controls/table-controls.component.ts

        ></ht-search-box>

+        <!-- Selects -->
+        <ht-select


Shouldn't the select dropdown be a multi select dropdown? For example: Show me all the APIs which lies in Service A or Service B or Service C

This is actually good feedback. I think multi select makes sense. That said, let me get the single select version out and demoed. I'll make the switch to IN filtering once everyone is onboard with the new changes.

yes. that makes sense

This is actually good feedback

What are you saying?!

…evel-apis

aaron-steinfeld · 2021-01-20T14:23:52Z

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts

+    const changedSelectFilter = selectFilterModel.getTableFilter(changed.value.value);
+
+    const selectFilters = [...existingSelectFiltersWithChangedRemoved, changedSelectFilter].filter(
+      f => f.value !== undefined


does the undefined filtering mean we can't switch back to unfiltered (isn't undefined the "All Services" option) ?

Correct. The select option returning a value of undefined signals to the widget to drop that filter, so we remove it from the request all together.

I'm confused - I would expect to not use that filter in the request, but it should still remain in the dropdown list if I want to return to see all apis across services. The behavior I read/you describe sounds like it is removed from the selection list once an option is chosen, never to return.

github-actions · 2021-01-20T18:07:59Z

Unit Test Results

    4 files ±0 231 suites ±0 12m 41s ⏱️ +48s
814 tests ±0 814 ✔️ ±0 0 💤 ±0 0 ❌ ±0
818 runs ±0 818 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit c5ae452. ± Comparison against base commit 4002603.

jake-bassett added 3 commits January 13, 2021 14:28

feat: top level apis navigation

bf48b24

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into top-l…

92fc0e8

…evel-apis

fix: linting

a786afb

jake-bassett requested a review from a team as a code owner January 13, 2021 22:41

jake-bassett commented Jan 13, 2021

View reviewed changes

projects/common/src/utilities/types/types.ts Show resolved Hide resolved

This comment has been minimized.

Sign in to view

anandtiwary reviewed Jan 13, 2021

View reviewed changes

aaron-steinfeld reviewed Jan 14, 2021

View reviewed changes

jake-bassett added 2 commits January 14, 2021 13:35

fix: clean up the select filter input code in table widget renderer

94ee8fd

fix: remove secondary entity spec and model

224d5fb

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Jan 15, 2021

View reviewed changes

...y/src/shared/dashboard/data/graphql/entity/attribute/entities-attribute-data-source-model.ts Show resolved Hide resolved

...ts/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts Show resolved Hide resolved

jake-bassett added 2 commits January 15, 2021 15:43

fix: change entities-attribute-data-source to return attributes array

281235f

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into top-l…

7c18fc0

…evel-apis

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Jan 16, 2021

View reviewed changes

skjindal93 reviewed Jan 18, 2021

View reviewed changes

jake-bassett added 2 commits January 19, 2021 13:40

Merge branch 'main' of github.com:hypertrace/hypertrace-ui into top-l…

b43054f

…evel-apis

fix: pr comments

16d27dc

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes Jan 20, 2021

View reviewed changes

jake-bassett merged commit c5ae452 into main Jan 20, 2021

jake-bassett deleted the top-level-apis branch January 20, 2021 18:03

Top level apis #499

Top level apis #499

Uh oh!

Conversation

jake-bassett commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaron-steinfeld Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jake-bassett Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

aaron-steinfeld Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jake-bassett commented Jan 13, 2021 •

edited

Loading

codecov bot commented Jan 13, 2021 •

edited

Loading

aaron-steinfeld Jan 15, 2021 •

edited

Loading

jake-bassett Jan 15, 2021 •

edited

Loading

aaron-steinfeld Jan 15, 2021 •

edited

Loading