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

Datasource Plugins: Allow tracking for configuration usage #72650

Merged
merged 6 commits into from
Aug 7, 2023

Conversation

sarahzinger
Copy link
Member

@sarahzinger sarahzinger commented Jul 31, 2023

What is this feature?

Sometimes we add new features/settings to a Datasource Plugin's configuration and we'd like to be able to track usage of these configuration settings.

This feature updates/adds to our Event Bus to publish when we save a datasource plugin.

Why do we need this feature?

We are working on improving our auth process for aws datasource plugins and it would be nice to collect some data about when and what auth type users are selecting in Grafana Cloud. Right now we generally only support the use of the "keys" auth type where users supply us with a set of long term credentials (although in other environments like OSS we support other methods as well). In the future we plan to offer more offerings here and would like to track their selection/usage.

To see an example of how plugins can use this feature, please see the Cloudwatch Example in this pr where we track auth type selection in the ConfigEditor on save.

I had originally explored adding this this auth type data directly to an existing event we have that fires off here. My challenge with that is I felt like I wanted to add datasource-specific information to that event and it felt a bit weird to publish a cloudwatch-specific property for all events.

Who is this feature for?

Datasource Plugin Developers who want to track save of a Datasource Configuration, and particular settings that may have been enabled/disabled

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@sarahzinger sarahzinger requested review from a team as code owners July 31, 2023 21:36
@sarahzinger sarahzinger requested review from mckn, joshhunt and L-M-K-B and removed request for a team July 31, 2023 21:36
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Jul 31, 2023
@sarahzinger sarahzinger modified the milestones: 10.2.x, 10.1.x Jul 31, 2023
@sarahzinger sarahzinger requested review from yaelleC, a team, fridgepoet and kevinwcyu and removed request for a team July 31, 2023 21:41
try {
await onUpdate({ ...dataSource });
trackDsConfigUpdated({ item: 'success' });
appEvents.publish(new DataSourceUpdatedSuccessfully());
appEvents.publish(new DataSourceUpdatedSuccessfully(eventData));
Copy link
Member

Choose a reason for hiding this comment

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

Does this generally publish the datasource configuration to anyone willing to listen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, anyone would be able to listen to these events (at least with the current version of the appEvents).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's my understanding as well. I think any subscriber that is set up while the page is loaded can access this information, so it's important that we keep this to be non-sensitive data.

Copy link
Member

@academo academo Aug 4, 2023

Choose a reason for hiding this comment

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

@sarahzinger we should consider than from a privacy and security perspective we should simply not expose anything from a plugin to another regardless of what we consider sensitive data or not.

Something you can do is to keep the state of the configuration in your plugin datasource and listen to the event (that triggers without sending any kind of payload) to later flush the data you already have. Broadcasting the plugin configuration all over the application is dangerous.

An alternative way is exposing an API that allows only the specific plugin to subscribe to the specific event for its plugin id and won't allow any other plugin to subscribe to the specific event. Not by means of obfuscation but checks in place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really great call outs @academo! Let me know what you think of this solution:

  • publish events without any associated data
  • then in the config component that has the state information we're looking for we subscribe and report there.

I think I was stuck thinking app event subscriptions had to happen in module.tsx since that's what I had seen us do elsewhere with other events, but in this case there's really no reason to!

Copy link
Member

@academo academo left a comment

Choose a reason for hiding this comment

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

@sarahzinger can a second plugin (plugin B) subscribe to this event and start getting the configuration data for the plugin that saved the information (plugin A) ?

Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

Great work on this! Just to understand a bit more of the use case. What kind of data do you want to add? Will it also be stored in the backend as part of jsonData? Is it other things than information related to the plugin meta data?

try {
await onUpdate({ ...dataSource });
trackDsConfigUpdated({ item: 'success' });
appEvents.publish(new DataSourceUpdatedSuccessfully());
appEvents.publish(new DataSourceUpdatedSuccessfully(eventData));
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, anyone would be able to listen to these events (at least with the current version of the appEvents).

@sarahzinger
Copy link
Member Author

@mckn Yes! Our use case is that we often have configuration settings that we create for datasource plugins that we'd like to have more understanding of usage for.

The example that is currently on our mind is that we're adding a new type of auth to the cloudwatch datasource. In Grafana Cloud right now we only support one type, but in OSS and other envs we have a range of auth types we support:
Screenshot 2023-08-03 at 12 22 23 PM

Once we add the new auth type to that dropdown we'd like to better understand who and how many people are opting into the usage of this feature, as well as get a sense of it's overall performance.

I had originally considered simply adding this auth type data to an existing reportInteraction call that we have here:
https://github.com/grafana/grafana/blob/main/public/app/features/datasources/state/actions.ts#L146

My thought there though was that then auth type would show up for every interaction even for plugins that are not relevant, which seemed a bit wrong. It also seemed a bit like an unsustainable pattern for every plugin to start adding their various configurations to one big object in core grafana (even if that plugin is not in core grafana).

My sense is that this is a fairly common use case where we have some kind of configuration settings in a datasource plugin that'd we'd love to learn more about that's fairly unique to that datasource plugin. Many of these configurations do not reveal any particular sensitive data. For example:

let's look at a configuration page for cloudwatch:
Screenshot 2023-08-03 at 1 16 29 PM

I'd love to know things like:

  • how many of our users have enabled linking between xray and cloudwatch
  • how many of our users have set a custom retry logic here for cloudwatch logs, etc.

I think by manually setting the data we'd like to track in a property in the datasource's jsonData we can make this generic enough to track particular fields, but ensure that we're not passing along the entire jsonData object which may or may not contain some kind of sensitive information.

Here's an example of how this might look in the plugin of us setting the property:
https://github.com/grafana/grafana-aws-sdk-react/pull/53/files#diff-9bd5c403af851469a03ce8274d95b0f68667afc08c9b8c4a595e1d11092ba111L88
You see that we both set Auth type in the jsonData but we also set it in the trackingData property. It's a bit clunky to have it twice but I think this helps reinforce a rule not to track sensitive data. Hopefully any actually sensitive data wouldn't be in JsonData anyway and would be in SecureJsonData but just in case this felt maybe a tad safer.

- no need to publish data from plugin
- add listeners in cloudwatch where state already is
- ensure that calls are made at the test not the save to more accurately measure if it works.
Copy link
Member

@academo academo left a comment

Choose a reason for hiding this comment

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

@sarahzinger much better with this approach thanks for doing the changes

@sarahzinger sarahzinger merged commit 6ac3348 into main Aug 7, 2023
18 checks passed
@sarahzinger sarahzinger deleted the sarahzinger/let-datasources-track-config branch August 7, 2023 12:31
academo pushed a commit that referenced this pull request Aug 11, 2023
Datasource Plugins: Allow tracking for configuration usage
aishyandapalli pushed a commit to aishyandapalli/grafana that referenced this pull request Aug 16, 2023
…2650)

Datasource Plugins: Allow tracking for configuration usage
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
…2650)

Datasource Plugins: Allow tracking for configuration usage
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants