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

[Uptime] Reimplement saved objects client creation #49979

Conversation

justinkambic
Copy link
Contributor

Summary

Resolves #49977.

When we implemented an interaction with Saved Objects for Uptime, we created our own client, when we should've utilized the request objects provided by the server. The request object has a getSavedObjectsClient function, which simplifies the access to a client (reduced boilerplate) as well as ensures we are interoperating correctly with other Kibana infrastructure features like Spaces.

This PR aims to decommission our custom client implementation and utilize the simpler getter strategy.

Testing

This change is working correctly if:

  1. The Uptime plugin loads correctly in Kibana
  2. Autocompletion continues working on the Kuery Bar

@justinkambic justinkambic added bug Fixes for quality problems that affect the customer experience technical debt Improvement of the software architecture and operational architecture v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v7.5.1 v7.4.3 labels Nov 1, 2019
@justinkambic justinkambic self-assigned this Nov 1, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic added release_note:skip Skip the PR/issue when compiling release notes v7.5.0 and removed v7.5.1 labels Nov 1, 2019
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up so quickly @justinkambic! I haven't pulled the code down yet, but I wanted to ask a couple of questions before diving in too deep

try {
return await this.savedObjectsClient.get('index-pattern', uptimeIndexPattern.id);
return await savedObjectsClient.get('index-pattern', uptimeIndexPattern.id);
Copy link
Member

Choose a reason for hiding this comment

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

Your previous implementation was always creating the index pattern in the default space, whereas the new implementation is creating the index pattern in whatever space the user happens to be in (same for retrieval). I'm not sure what the contents of this index pattern are, but is this something you want/expect users to be able to edit? Are most uptime users aware this index pattern exists, or is it an implementation detail?

Since the end-user is the one creating the index pattern now, you will need to update your feature privileges to denote that users need the ability to write index patterns:

xpack.registerFeature({
id: PLUGIN.ID,
name: i18n.translate('xpack.uptime.featureRegistry.uptimeFeatureName', {
defaultMessage: 'Uptime',
}),
navLinkId: PLUGIN.ID,
icon: 'uptimeApp',
app: ['uptime', 'kibana'],
catalogue: ['uptime'],
privileges: {
all: {
api: ['uptime'],
savedObject: {
all: [],
read: [],
},
ui: ['save'],
},
read: {
api: ['uptime'],
savedObject: {
all: [],
read: [],
},
ui: [],
},
},
});

question: What happens when a readonly user attempts to create this index pattern? The operation will fail, but will Uptime be resilient enough to still function for them?

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

The way you're using the savedObjectsClient looks good.

Just be aware that this PR will now store an index pattern object in each space whereas before there would have been only one object across all spaces. Since it seems to always be a static object from ./heartbeat_index_pattern.json, it probably doesn't matter. Having an object in each space will be more flexible if you want to allow users to change the index pattern without affecting other spaces in the future.

@rudolf
Copy link
Contributor

rudolf commented Nov 1, 2019

@legrego beat me to it!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic
Copy link
Contributor Author

@rudolf @legrego thanks for the feedback!

So in this case, we don't currently expect the users to need to edit/modify the index pattern. It's a static resource we're using just for autocomplete purposes, we may need to revisit this in the future though.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member

legrego commented Nov 18, 2019

So in this case, we don't currently expect the users to need to edit/modify the index pattern. It's a static resource we're using just for autocomplete purposes, we may need to revisit this in the future though.

Thanks for clarifying @justinkambic. This is an interesting use-case...since we don't expect end-users to update this, I don't think it makes sense to have the index pattern created by them either. The resulting privilege model would be confusing for administrators ("why does Uptime grant access to all of my index patterns?").

Given that, I think it makes sense to have the internal user still create this saved object, rather than the end-user.

Aside: Do you know if the Kuery Bar coupled to index patterns today? Do you see a future where we could implement the autocomplete functionality without index patterns? As an outsider with no knowledge of these systems, it seems strange that Uptime needs to create an index pattern to enable autocomplete functionality.

@justinkambic
Copy link
Contributor Author

justinkambic commented Nov 18, 2019

@legrego thanks for getting back to me on this 👍

Do you know if the Kuery Bar coupled to index patterns today?
It does require an index pattern today. Here's an example from APM's implementation.

They also are fetching index pattern(s) associated with their app.

it seems strange that Uptime needs to create an index pattern to enable autocomplete functionality.

The difference is we haven't added the ability to configure index patterns yet for Uptime. It's something we've been putting off but something we are looking to do very soon now that we are working toward supporting CCS. The solution we're seeing here has always been intended as temporary and it's not lost on me how weird it is.

Given that, I think it makes sense to have the internal user still create this saved object, rather than the end-user.

I'm ++ on this as well but not 100% clear on what I need to do to make it happen.

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@legrego
Copy link
Member

legrego commented Nov 18, 2019

Given that, I think it makes sense to have the internal user still create this saved object, rather than the end-user.

@rudolf I think I found a use-case for the internally-scoped SOC that you wanted to avoid 😄

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic
Copy link
Contributor Author

We don't need this PR anymore, as the issue has been resolved by #51403 and we're now using new platform Saved Objects client in a space-aware way.

@justinkambic justinkambic deleted the uptime_remove-custom-saved-objects-client branch December 19, 2019 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability technical debt Improvement of the software architecture and operational architecture v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Saved Objects interaction is performed with an unnecessary custom client
4 participants