Skip to content
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

fix: revert the way we do ds lookups #911

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

ckbedwell
Copy link
Contributor

@ckbedwell ckbedwell commented Aug 19, 2024

Problem

Background context

Synthetic Monitoring relies on two supporting databases to work at full capacity: a Grafana Cloud-hosted prometheus database and a Grafana Cloud-hosted Loki database.

A Grafana Cloud instance automatically provisions two datasource 'connectors' which connect to each respectively, confusingly referred to as a Prometheus Datasource and a Loki Datasource. In theory these should all remain stable, are named predictably and are immutable.

The problem which lies with the plugin is that it based on certain assumptions which may be untrue in certain situations (it is worth noting they are rare but do and will continue to exist and some clients specifically ask to bypass these assumptions):

  • That the Synthetic Monitoring tenant is using the default set-up
    • This assumption becomes incorrect if someone were to update their Synthetic Monitoring database provisioning file
  • That the provisioning jsonData for the Synthetic Monitoring Datasource is the source of truth for these connections.
    • This assumption becomes incorrect if someone has utilised the SM api /api/v1/tenant/update route to reassign different instances to associate with
    • This assumption also becomes incorrect if the default provisioned datasources have been renamed in some way (i.e. altering their provisioning files)

The next problem that the Synthetic Monitoring plugin faces is that the metrics and logs api are queried via their datasources within Grafana. We query datasources by their UIDs, however the SM tenant endpoint (which is the real source of truth) does not concern itself with associating with datasources but with Grafana Cloud-hosted database instances.

The api can provide the hostedId of the database instances it is writing to but it has no knowledge of what datasources a Grafana Cloud instance has which it uses to query against.

If we do not wish to rely on the assumptions above to know what datasources we can utilise it is currently up to to the plugin to ask the SM tenant endpoint and search through a user's available datasources and ask for the hostedIds of each (in this case the basicAuthUser === hostedId). It is worth noting this may not always be reliable because a user may have access to multiple datasources which share the same hostedId but have different access policies.


Actual problem

In the previous update we worked on the assumption that somewhere during the Grafana Cloud bootup that the datasources provided by getDatasourceSrv.getList() was mutating the provisioning data and doing the cross-referencing for us. See below in our development environment where the jsonData provided by the SM /settings endpoint omits the uid but it is present when querying the Synthetic Monitoring jsonData provided by getDatasourceSrv.getList()

Screenshot of the browser dev tools. It shows a the settings network request response alongside the console data.

It appears this new assumption is wrong and that not all instances have this data available.

Solution

Unfortunately the solution for now is to go back to the original assumptions that the provisioning data provided by the SM plugin is correct and do the datasource look ups utilising this information.

A more thorough solution will be explored in future work.

@ckbedwell ckbedwell requested a review from VikaCep August 19, 2024 16:44
return linkedDS;
}
}
return config.datasources[linkedDSInfo.grafanaName];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a fallback, but did you mention it was not reliable to find the datasource by the grafanaName? should we add a comment to this line indicating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I've added a comment in above the datasource methods and linked to the description I've now filled out in this PR.

@VikaCep
Copy link
Contributor

VikaCep commented Aug 19, 2024

Looks good to me! Tested it locally and couldn't find any issues.

@ckbedwell ckbedwell marked this pull request as ready for review August 20, 2024 09:50
@ckbedwell ckbedwell requested a review from a team as a code owner August 20, 2024 09:50
@ckbedwell ckbedwell merged commit 68bc542 into main Aug 20, 2024
5 checks passed
@ckbedwell ckbedwell deleted the fix/regress-to-assumed-ds-lookup branch August 20, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants