-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(ourlogs): Fix logs widget viewer link #98126
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
Conversation
Should say 'open in explore' and link to the correct logs page
interval: queryParams.interval, | ||
mode: queryParams.mode, | ||
}); | ||
} |
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.
Bug: Incorrect Parameter Types and Missing Sort
When calling getLogsUrl
, the aggregateFields
parameter receives queryParams.visualize
, which is an array of visualization objects, instead of the expected array of aggregate field strings. This type mismatch may cause incorrect logs URLs. Additionally, the sort
parameter from queryParams
is omitted, leading to lost sorting functionality.
const traceItemDataset = getTraceItemDatasetFromWidgetType(widget.widgetType); | ||
|
||
if (widget.queries.length > 1) { | ||
if (traceItemDataset === TraceItemDataset.LOGS) { | ||
Sentry.captureException( | ||
new Error( | ||
`getWidgetExploreUrl: multiple queries for logs is unsupported, widget_id: ${widget.id}, organization_id: ${organization.id}, dashboard_id: ${widget.dashboardId}` | ||
) | ||
); | ||
} | ||
return _getWidgetExploreUrlForMultipleQueries( | ||
widget, | ||
dashboardFilters, | ||
selection, | ||
organization | ||
organization, | ||
traceItemDataset | ||
); |
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.
Potential bug: Logs widgets with multiple queries incorrectly generate a URL for the traces compare view (/explore/traces/compare/
) instead of the logs view, misdirecting users.
-
Description: When a dashboard widget for logs contains multiple queries, the
getWidgetExploreUrl
function calls_getWidgetExploreUrlForMultipleQueries
. This function, in turn, usesgetExploreMultiQueryUrl
, which is hardcoded to always generate a URL for the traces compare view (/explore/traces/compare/
). As a result, clicking 'Open in Explore' on a multi-query logs widget incorrectly navigates the user to the traces comparison tool instead of the logs exploration view, disrupting the user's workflow and leading them to the wrong context. -
Suggested fix: The
_getWidgetExploreUrlForMultipleQueries
function should be updated to handle different dataset types. It should check the widget's dataset and call the appropriate URL generation function, such asgetLogsUrl
for logs widgets, instead of always defaulting togetExploreMultiQueryUrl
which is specific to traces.
severity: 0.65, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
default: | ||
return TraceItemDataset.SPANS; // Default to spans for backwards compatibility |
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.
nit: Can we force the caller to handle the backwards compatibility with widgetType ?? WidgetType.SPANS
? and throw an error here? This feels like we're likely to introduce subtle bugs in the future.
### Summary Should say 'open in explore' and link to the correct logs page #### Screenshots <img width="1145" height="555" alt="Screenshot 2025-08-22 at 12 03 12 PM" src="https://github.com/user-attachments/assets/1750a00f-7022-4dcc-ba25-115958958776" />
Summary
Should say 'open in explore' and link to the correct logs page
Screenshots