-
Notifications
You must be signed in to change notification settings - Fork 41
[FEATURE]: add transforms to the logs table #529
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.
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
base: main
Are you sure you want to change the base?
Conversation
48a3ca0 to
9d07767
Compare
| const { onChange, value } = props; | ||
|
|
||
| const handleTransformsChange: TransformsEditorProps['onChange'] = (transforms) => { | ||
| console.log(transforms); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| console.log(transforms); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
9d07767 to
5610fc9
Compare
|
sure, but why don't do the fix first so we can actually test this is working? |
Hmm, yes why not. Let's do the fix first. |
|
I just shared with you my testing of the feature, that's why I found the issue... There's probably a lack of testing for this, as this used to be working before. |
Signed-off-by: Mahmoud <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com>
5610fc9 to
dda2b60
Compare
@jgbernalp I would to mention that from what I understood the Transform has not been broken with a recent change. Why doesn't it work?We can not see the effect of the transform for a Log Table because it has never been used and designed for log table! The data structure doesn't fitThe data transform utility does not recognize log entry structure. And what is really a column in a log entry table? /* This is the utility */
export function transformData(
data: Array<Record<string, unknown>>,
transforms: Transform[]
): Array<Record<string, unknown>> => {/* ... */}/* This is the log structure */
/* There is column here */
export interface LogEntry {
timestamp: number;
line: string;
labels: Labels;
}/* If you try the wrong usage, it throws an exception. (Type mismatch) */
const rawLogs = queryResults
.flatMap((result) => result?.data.logs?.entries ?? [])
.sort((a, b) => b.timestamp - a.timestamp);
const logs = useMemo(() => transformData(rawLogs, spec.transforms ?? []), [rawLogs, spec.transforms]);Likely steps
|
|
Transformation principe has been thought to be re-used in any query type, but yeah currently there are only TimeSeriesQuery transformations, that will probably not work/make sense with logs or other query types. I wonder if we should not create a new type of plugin (Transformation), so datasources can add their own transformations and people with specific needs can add their own too 🤔 |
|
IMO transforms should apply to a table data, regardless of the QueryType behind. When the data is on the table it has already been transformed from query type to Record of things. Maybe we should re check if this should be the case, but I think it does not make sense to force currently the logs to match the timeseries transforms... |
I think the name Logs Table is misleading. What we see is not the table/table view. https://grafana.com/whats-new/2023-12-13-logs-table-ui/ Second 27 What is a Log Table ViewA table view turn the logs into a structure suitable for rendering in a table. The transforms can be applied to the table view. Steps
|
I agree with this.
I did not really use or check Log Table panel, so I can't tell what is the best solution |
|
@shahrokni Not sure If understand correctly. What is the scope of the work you are planning to do? Have a button to switch from logs table to a generic table? |



Relates to perses/perses#3786
Description 🖊️
This PR will add the current transform feature to the logs table.
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes