-
Notifications
You must be signed in to change notification settings - Fork 21
fix: use configured datasource instead of default one #1470
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
Script size changes
Totals
|
4620236 to
8ed0447
Compare
That sentence I think indicates that, if we want to support this, we probably need to do it at a higher level. If we want to support this, we probably need to allow the user to specify the data source configuration, that will have to be stored in backend, and the plugin should consume whatever is set there. Additionally this should be propagated to HG somehow, so resetting the plugin does not revert the configuration. |
I agree, this PR was aimed specifically to address the use case of https://github.com/grafana/support-escalations/issues/19505. They were previously taking advantage of a workaround in order to consume the LBAC datasource and it stopped working, so this solution was a temporary fix. (see https://raintank-corp.slack.com/archives/C0162C1K9E1/p1763384093931469) I do agree the right and long-term solution is to explicitly allow users to select the desired datasources in the UI and store it, so it doesn't reset when the app is reinstalled or the SM datasource restored, but as you said, that would have to be triaged and treated as a new feature. We can close this PR if we want to take that road. |
|
@VikaCep any update on the fix? From the perspective of anyone using Synthetics this is a regression and should be fixed. Long term improvement sounds good, but let's address restoring a working version first 🙏 |
|
@ar2pi this PR is still up for review, and while it does try address the customer’s immediate issue, there are some inconsistencies in other layers of the architecture that we should consider as well, as Daniel pointed out. The customer was relying on a workaround that we had previously confirmed was not something we officially support, so I think it’s debatable whether this qualifies as a regression:
|
|
@VikaCep This absolutely counts as a regression from a user perspective. For context: Synthetics logs worked fine for users who rely on a LBAC datasource for logs. Now, it doesn't. Could we think in terms of win-win here? Have the temporary fix asap, restore user satisfaction. Then think longer term. |
ckbedwell
left a comment
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.
Let's just do it. What's the worst that can happen? 😬
It'll be up to the user to manage this properly and we can look at supporting this more officially in the future.
Ref https://github.com/grafana/support-escalations/issues/19505
Related to #911
Support custom Loki datasources in Synthetic Monitoring
Problem
The Synthetic Monitoring app hardcoded fallbacks to
uid: 'grafanacloud-logs'for log queries, preventing users from configuring alternative Loki datasources. This blocked use cases where customers need to use different Loki datasources.Affected areas:
grafanacloud-logsUID firstSolution
Updated the datasource resolution logic to respect the configured datasource in
jsonData.logsbefore falling back to hardcoded defaults.Changes:
DataSource.ts-getLogsDS()andgetMetricsDS()jsonDatafirstuseAppInitializer.ts-findDatasourceByNameAndUid()Testing
To test:
jsonData.logsNotes
GCOM Provisioning Required:
If the SM datasource is deleted and re-initialized, it will reset to whatever GCOM provisions (the default datasource).
Future Enhancement:
We may evaluate adding a UI dropdown in the plugin configuration to allow users to select from available Loki datasources, removing the dependency on GCOM provisioning for this use case.