Logs: Deprecated showContextToggle in DataSourceWithLogsContextSupport
#77232
Conversation
matyax
requested review from
iwysiu and
idastambuk
and removed request for
a team
matyax
force-pushed
the
matyax/deprecate-show-context-toggle
branch
from
e85b117
to
7b963e5
Compare
|
Backend code coverage report for PR #77232 |
|
Frontend code coverage report for PR #77232
|
|
I am not sure we want to do this? |
Does it have a valid use case or we would be keeping it "just in case"? Personally, I'm not a fan of keeping code for "just in case we need it" purposes, and having 3 data sources returning a harcoded |
|
@ivanahuckova what do you think? Keep? Deprecate? |
|
@matyax have you checked (maybe trough github search) if any other plugin uses it? If not and we don't see any reason to have it, I wouldn't be opposed to deprecating it. |
|
There doesn't seem to be any other data source implementing it https://github.com/search?q=+showContextToggle%28&type=code |
|
|
||
| return false; | ||
| showContextToggle = (row: LogRowModel): boolean => { | ||
| return hasLogsContextSupport(this.props.datasourceInstance); | ||
| }; |
There was a problem hiding this comment.
Technically, we should not change this because we are only deprecating showContextToggle and not removing it. But as we found out that there are not plugins using it, I THINK it should be fine.
But we should definitely update plugins documentation and remove showContextToggle from there https://github.com/grafana/plugin-tools/blob/41a70bffa66ef0b22a690ac0a7b687fe7ec5264f/docusaurus/docs/tutorials/build-a-logs-data-source-plugin.md?plain=1#L400. And also remove it from https://github.com/grafana/grafana-plugin-examples/blob/main/examples/datasource-logs/src/datasource.ts#L101.
There was a problem hiding this comment.
Good call!
grafana/plugin-tools#502 && grafana/grafana-plugin-examples#215
matyax
changed the title
showContextToggle in DataSourceWithLogsContextSupport is now deprecatedshowContextToggle in DataSourceWithLogsContextSupport
|
@svennergr Any concern with these changes? |
Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>
matyax
force-pushed
the
matyax/deprecate-show-context-toggle
branch
from
401a5da
to
ec9a0df
Compare
| @@ -156,10 +156,6 @@ export class CloudWatchDatasource | |||
| ); | |||
| } | |||
|
|
|||
| showContextToggle() { | |||
There was a problem hiding this comment.
@grafana/aws-datasources FYI.
…ort (grafana#77232) * Logs: deprecate showContextToggle * Logs: make row mandatory on showContextToggle prop * DataSourceWithLogsContextSupport: make showContextToggle optional * Loki: update test * Update packages/grafana-data/src/types/logs.ts Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> * Prettier * showContextToggle: update deprecation --------- Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>
This function used to control the visibility of the show context button, but that has changed and it was now returning a hardcoded true.
Currently, the method to identify log data sources that support Logs Context is to check if they implement
DataSourceWithLogsContextSupportthrough thehasLogsContextSupportguard. Without implementing this interface it's impossible to support the feature, thus this is the way data sources indicate if the option should be available or not.Additionally, made the row argument mandatory, first because it was being sent by the caller (LogRowMessage), but more importantly because it helps to provide support for Logs Context in mixed data sources. See #76623
Deprecation notice
Since Grafana 10.3 we're deprecating the
showContextToggledata source method. To signal support of Logs Context, it is enough to implement theDataSourceWithLogsContextSupportinterface.Which issue(s) does this PR fix?:
Fixes #66819
Related with #73568 and #73565
Special notes for your reviewer:
There should be no function change with this deprecation.