feat: (LogPanelTable) - sync the display fields and urlColumns between the logs panel and table #1189
Conversation
|
Overall looking good and working as expected, I really like the idea of being able to switch more easily between the views without needing to reconfigure columns. A few bugs/regressions with the log line and time columns to clean up and we should be good to go: If I have nothing visualized, and I toggle the table and back to the logs panel, the log line is stripped out and I see the "show original log line" button. I would expect this to be a noop. base-case.movThe timestamp currently is always displayed in the logs panel, so it's a bit jarring to have it removed from the Table: When taking a logs panel query to Explore, the Time column is always stripped out, which doesn't occur on latest main: |
TY! Let me know if you think this is a better fix. Basically I'm storing default columns for 'body' and 'timestamp' Screen.Recording.2025-04-14.at.10.08.13.AM.mov |
|
@L2D2Grafana that is much closer to what I expect, except at the [0:29-0:33] timestamp in your video, I would expect the panel in Explore to have the same columns as the panel in Logs Drilldown |
|
…n the logs panel and table
L2D2Grafana
force-pushed
the
l2d2/1126-sync-logs-table
branch
from
4df791f to
7390701
Compare
| onActivateSyncDisplayedFieldsWithUrlColumns = () => { | ||
| const searchParams = new URLSearchParams(locationService.getLocation().search); | ||
| const urlColumnsParam = searchParams.get('urlColumns'); | ||
| const urlColumnsUrl = urlColumnsParam ? (JSON.parse(decodeURIComponent(urlColumnsParam)) as string[]) : null; |
There was a problem hiding this comment.
This seems like a good candidate to use https://github.com/grafana/logs-drilldown/blob/main/src/services/narrowing.ts that was introduced by @gtk-grafana to have more confidence on the actual type of stuff like user input, parsed json, etc.
We could add a narrowArray(type: string) or narrowStringArray().
There was a problem hiding this comment.
Updated! Interesting concept, I took a look they all kind of seem like type validations. I wonder about naming them isValidType or isEmpty.
There was a problem hiding this comment.
Definitely. Not a huge fan of the "narrow" prefix. ´assert[Type]´ could also be an option.
|
This is super cool! It makes a huge difference going back and forth between visualizations. |
src/services/narrowing.ts
Outdated
| @@ -123,4 +123,19 @@ export function narrowFilterOperator(op: string): LabelFilterOp | NumericFilterO | |||
| } | |||
| } | |||
|
|
|||
| export function narrowUrlParam(param: unknown): string[] | null { | |||
There was a problem hiding this comment.
Excellent! My only suggestion would be to make it generic so we can reuse this narrowing elsewhere, maybe like:
| export function narrowUrlParam(param: unknown): string[] | null { | |
| export function narrowStringsArray(param: unknown): string[] | null { |
I wanted to make it generic but I couldn't find an easy way.
There was a problem hiding this comment.
Yeah if there was a generic narrowing strategy it would be the most imported typescript npm package, I think this is something that needs a bit of hand-holding
There was a problem hiding this comment.
there is a unknownToStrings method in narrowing already tho
There was a problem hiding this comment.
I tried unknownToStrings but it's a slightly different use case where the param are passed in as strings and not an array so it never gets past
if (Array.isArray(a)) {
There was a problem hiding this comment.
Updated the func name.
Screen.Recording.2025-04-10.at.3.59.25.PM.mov