feat: Add Table Notices #957

Golodhros · 2021-03-18T20:59:26Z

Summary of Changes

Adds Table and Dashboards notifications in the detail page using the current JavaScript configuration system

Implements amundsen-io/rfcs#29

Tests

Tested configuration helper

Documentation

Not yet

mgorsk1 · 2021-03-19T07:22:12Z

Cool to see that one make it through ! We've been meaning to work on something similar next months.

Regarding questions I think I might have input for 1 and 2 - our use case assumed not only alerts but also warnings and info messages. Examples information we wanted to show were:

1. This is an official table, access it using view X (info)
2. This table was not used in last 90 days and might be deprecated (warning)
3. There is a new version of this table named Y (alert - like in your example)
4. Other users has also viewed tables X, Y, Z (based on recommendation engine output) (info)

so it definitely would be more scalable with more than just one type of notification in my opinion (and possibility to define more than one notification per table)

One nit - would it make sense to have alerts section under the description ?

Golodhros · 2021-03-19T15:49:17Z

Cool to see that one make it through ! We've been meaning to work on something similar next months.

Regarding questions I think I might have input for 1 and 2 - our use case assumed not only alerts but also warnings and info messages. Examples information we wanted to show were:
1. This is an official table, access it using view X (info)
2. This table was not used in last 90 days and might be deprecated (warning)
3. There is a new version of this table named Y (alert - like in your example)
4. Other users has also viewed tables X, Y, Z (based on recommendation engine output) (info)
so it definitely would be more scalable with more than just one type of notification in my opinion (and possibility to define more than one notification per table)

One nit - would it make sense to have alerts section under the description ?

That's awesome, thanks for chiming in @mgorsk1!

I could easily add the three types of notification part, it think it would be something like:

'schema.name': {
  severity: NotificationSeverity.ALERT,
  message: 'Lorem ipsum....'
},
'schema.name2': {
  severity: NotificationSeverity.INFO/WARNING,
  message: ...
}]

Regarding the positioning, let me ask our designer to see what she thinks. I guess the placement was because they wanted to give it the most importance.

What do you think about the RFC, should I open one?

dorianj · 2021-03-19T16:08:57Z

This is cool! I haven't looked closely, but very much want this functionality.

Minor nit: is there something else we could name this? Notifications to me implies stuff getting pushed to people. Perhaps table notices, alerts, or something like that?

+1 on three types of notification part -- I think having severity classes is useful

I think that it would be reasonable to have multiple notifications per table?

regarding RFC, I think this does qualify, it is a new feature. The biggest discussion points I see are:

whether this should live in the graph, or configuration? (or possibly a mix -- might be reasonable to have a design where
should the configuration, or graph, allow for wildcard associations? Some people might want to add notices for all tables in a certain namespace, for example
whether this should be associable with all object types, or just tables?

it is quite probable the initial implementation wouldn't do the full scope, but the initial impl details may change based on some of these answers

Golodhros · 2021-03-22T20:58:52Z

This is cool! I haven't looked closely, but very much want this functionality.

Minor nit: is there something else we could name this? Notifications to me implies stuff getting pushed to people. Perhaps table notices, alerts, or something like that?

Makes sense, changing it to "Notices"

+1 on three types of notification part -- I think having severity classes is useful

Updated this PR with it

I think that it would be reasonable to have multiple notifications per table?

This one is a bit more problematic, as we would need to limit them and maybe offer the option to close them?

regarding RFC, I think this does qualify, it is a new feature. The biggest discussion points I see are:

I will create an RFC now.

whether this should live in the graph, or configuration? (or possibly a mix -- might be reasonable to have a design where

should the configuration, or graph, allow for wildcard associations? Some people might want to add notices for all tables in a certain namespace, for example

whether this should be associable with all object types, or just tables?

it is quite probable the initial implementation wouldn't do the full scope, but the initial impl details may change based on some of these answers

All legit questions.
I will add the association to Dashboards and People too. The others I think they are interesting though.

danwom · 2021-03-30T23:49:20Z

docs/application_config.md

+          },
+      },
+    },
+  },


A few notes about this configuration:

I think this furthers the need to move our configs to an external config service. Updating these messages will require a rebuild and redeploy which is not ideal.

Adding notices to our datastore seems like it could work, but with a lot more work. (adding to graph model, databuilder jobs, user editing/adding notices, etc) Doesn't seem worth it at this point.

Using the "<SCHEMANAME>.<TABLENAME>" as the table identifier seems somewhat nonstandard. Everywhere we identify a table we always use the pattern {database}://{cluster}.{schema}/{table_name}. Is the intent here to match similar tables in different databases or clusters?

nit: rename message to messageHtml. Also is it required to wrap the message in a span?

A few notes about this configuration:

I think this furthers the need to move our configs to an external config service. Updating these messages will require a rebuild and redeploy which is not ideal.

100% Agree, I think a combination of a feature flag system plus an admin UI would be ideal.

Using the "<SCHEMANAME>.<TABLENAME>" as the table identifier seems somewhat nonstandard. Everywhere we identify a table we always use the pattern {database}://{cluster}.{schema}/{table_name}. Is the intent here to match similar tables in different databases or clusters?

Oh, didn't think about this. I will try to include the rest.

nit: rename message to messageHtml. Also is it required to wrap the message in a span?

Good call, I that key name will be more explicit.
I will check about the span, I can't remember why it was there.

amundsen_application/static/js/components/Alert/index.tsx

danwom · 2021-03-31T00:00:39Z

amundsen_application/static/js/pages/DashboardPage/index.tsx

    const hasDescription =
      dashboard.description && dashboard.description.length > 0;
    const hasLastRunState =
      dashboard.last_run_state && dashboard.last_run_state.length > 0;
+    const dashboardNotice = getResourceNotices(
+      ResourceType.dashboard,
+      `${dashboard.product}.${dashboard.name}`


I don't think the product and dashboard name are enough to uniquely identify a dashboard. Multiple dashboards in the same product are named some generic "payments dashboard" or something that would all match the same config value. The "uri" ("key" for tables, we should standardize to one or the other but that's a different issue) for dashboards is {product}://{cluster}.{group}/{name}.

Good call, I will try to add more stuff here.

danwom

Aside from changing the key/uri format, should be good to merge.

amundsen_application/static/js/pages/DashboardPage/index.tsx

amundsen_application/static/js/pages/TableDetailPage/index.tsx

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

…cs and tests updates Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

allisonsuarez · 2021-04-02T15:31:42Z

amundsen_application/static/js/components/Alert/index.tsx


 import './styles.scss';

-const STROKE_COLOR = '#b8072c'; // $red70
+const SEVERITY_TO_COLOR_MAP = {
+  [NoticeSeverity.INFO]: '#3a97d3', // cyan50


We don't have these colors on the _colors css file as variables?

Yeah, that's the 'cyan50', but we cannot bring the scss variables into the JS files :(

allisonsuarez

LGTM with one small comment

* Alert styling on Table Detail page Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Sources table notification from the config object Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Adds severities to notices Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Adding it to dashboard Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Updating for dashboard notices as well Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Update betterer results Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Adding documentation Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com> * Unique table and dashboard identifications, messageHtml as key and docs and tests updates Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Golodhros requested review from allisonsuarez and danwom March 18, 2021 21:15

Golodhros force-pushed the mi-table-notifications branch from 0b009eb to 38e66dd Compare March 22, 2021 22:28

Golodhros mentioned this pull request Mar 22, 2021

Table and Dashboard Notices amundsen-io/rfcs#29

Merged

Golodhros force-pushed the mi-table-notifications branch 2 times, most recently from 98e3849 to 17fe3ed Compare March 22, 2021 22:30

Golodhros changed the title ~~feat: WIP Add Table Notifications~~ feat: Add Table Notifications Mar 23, 2021

Golodhros marked this pull request as ready for review March 23, 2021 21:51

Golodhros requested review from dikshathakur3119, dorianj, feng-tao and ttannis as code owners March 23, 2021 21:51

Golodhros requested a review from a team as a code owner March 30, 2021 17:15

Golodhros force-pushed the mi-table-notifications branch from 8493516 to 2e77116 Compare March 30, 2021 17:16

danwom reviewed Mar 31, 2021

View reviewed changes

Golodhros force-pushed the mi-table-notifications branch from afb22c7 to 0d73c63 Compare March 31, 2021 17:19

Golodhros changed the title ~~feat: Add Table Notifications~~ feat: Add Table Notices Mar 31, 2021

danwom reviewed Mar 31, 2021

View reviewed changes

amundsen_application/static/js/pages/DashboardPage/index.tsx Show resolved Hide resolved

amundsen_application/static/js/pages/TableDetailPage/index.tsx Show resolved Hide resolved

Golodhros added 8 commits April 1, 2021 10:07

Alert styling on Table Detail page

4264539

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Sources table notification from the config object

a892bcb

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Adds severities to notices

c8091ba

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Adding it to dashboard

5007622

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Updating for dashboard notices as well

1772245

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Update betterer results

8578a02

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Adding documentation

587614f

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Unique table and dashboard identifications, messageHtml as key and do…

c5b7f6c

…cs and tests updates Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Golodhros force-pushed the mi-table-notifications branch from 0d73c63 to c5b7f6c Compare April 1, 2021 17:12

danwom approved these changes Apr 1, 2021

View reviewed changes

allisonsuarez reviewed Apr 2, 2021

View reviewed changes

allisonsuarez approved these changes Apr 2, 2021

View reviewed changes

Golodhros merged commit e3be638 into master Apr 2, 2021

Golodhros deleted the mi-table-notifications branch April 2, 2021 16:02

stswn mentioned this pull request Jun 21, 2021

feat: Adds dynamic table notices from Atlas amundsen-io/amundsen#1254

Closed

Golodhros mentioned this pull request Jun 24, 2021

feat: Making notices more general amundsen-io/amundsen#1274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Table Notices #957

feat: Add Table Notices #957

Golodhros commented Mar 18, 2021 •

edited

mgorsk1 commented Mar 19, 2021 •

edited

Golodhros commented Mar 19, 2021

dorianj commented Mar 19, 2021

Golodhros commented Mar 22, 2021

danwom Mar 30, 2021

Golodhros Mar 31, 2021

danwom Mar 31, 2021

Golodhros Mar 31, 2021

danwom left a comment

allisonsuarez Apr 2, 2021

Golodhros Apr 2, 2021

allisonsuarez left a comment

feat: Add Table Notices #957

feat: Add Table Notices #957

Conversation

Golodhros commented Mar 18, 2021 • edited

Summary of Changes

Tests

Documentation

mgorsk1 commented Mar 19, 2021 • edited

Golodhros commented Mar 19, 2021

dorianj commented Mar 19, 2021

Golodhros commented Mar 22, 2021

danwom Mar 30, 2021

Choose a reason for hiding this comment

Golodhros Mar 31, 2021

Choose a reason for hiding this comment

danwom Mar 31, 2021

Choose a reason for hiding this comment

Golodhros Mar 31, 2021

Choose a reason for hiding this comment

danwom left a comment

Choose a reason for hiding this comment

allisonsuarez Apr 2, 2021

Choose a reason for hiding this comment

Golodhros Apr 2, 2021

Choose a reason for hiding this comment

allisonsuarez left a comment

Choose a reason for hiding this comment

Golodhros commented Mar 18, 2021 •

edited

mgorsk1 commented Mar 19, 2021 •

edited