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

Update VisLayer data models #3374

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Feb 2, 2023

Description

This PR updates the data models for the VisLayers such that we are passing sufficient information to the 'View Events' flyout to render it correctly. Details on the flyout & its mocks are in the related issue #3373

Note there will still be some added field(s) related to error handling - details on that as part of issue #3267

Issues Resolved

Closes #3373

Check List

  • All tests pass
  • Commits are signed per the DCO using --signoff

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
type: PluginResourceType;
id: string;
name: string;
urlPath: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be an example of this urlPath look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be something like anomaly-detection-dashboards#/detectors/<detector-id>/configurations. The base path would come from core services.

Full example:

<EuiLink href={`${getCore().http.basePath.prepend(`${urlPath}`)}`}>{name}</EuiLink>

Copy link
Member

Choose a reason for hiding this comment

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

@ohltyler Is there an updated design doc for these data models? Mostly just want to make sure our docs stay in sync with these changes that come out of the actual implementation. We also just may want to have a placeholder issue to remind us to document example usage (you may have already created one).

Copy link
Member

Choose a reason for hiding this comment

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

Is it enough for urlPath to be a static string? In your example, it seems like maybe you'd want to be able to pass something like a detector-id?

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

A couple minor questions, but otherwise looks good.

type: PluginResourceType;
id: string;
name: string;
urlPath: string;
Copy link
Member

Choose a reason for hiding this comment

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

@ohltyler Is there an updated design doc for these data models? Mostly just want to make sure our docs stay in sync with these changes that come out of the actual implementation. We also just may want to have a placeholder issue to remind us to document example usage (you may have already created one).

type: PluginResourceType;
id: string;
name: string;
urlPath: string;
Copy link
Member

Choose a reason for hiding this comment

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

Is it enough for urlPath to be a static string? In your example, it seems like maybe you'd want to be able to pass something like a detector-id?

@ohltyler
Copy link
Member Author

ohltyler commented Feb 4, 2023

@ohltyler Is there an updated design doc for these data models? Mostly just want to make sure our docs stay in sync with these changes that come out of the actual implementation. We also just may want to have a placeholder issue to remind us to document example usage (you may have already created one).

I'm working on updating the linked View Events flyout issue with details #3155 . From there will have some deeper explanation on these added fields. Is that sufficient? I'm only trying to merge this earlier so there is less interference / less data model changes down the road as plugins integrate.

Is it enough for urlPath to be a static string? In your example, it seems like maybe you'd want to be able to pass something like a detector-id?

I've thought about this, my one concern is I would like to keep it flexible enough for other potential ways a plugin would like to link something. For example, a resource ID may not be necessary (or possible) as part of their url, or maybe they want it to link something related to the resource, but have some different ID there, etc. If we leave as a string it adds no coupling with a resource ID in the url.

@ohltyler
Copy link
Member Author

ohltyler commented Feb 4, 2023

I'll also open a base meta issue regarding documentation / usage / how to onboard & add it to the feature meta issue.

Update: see #3383

@joshuarrrr joshuarrrr merged commit a990ab9 into opensearch-project:feature/feature-anywhere Feb 4, 2023
@ohltyler ohltyler deleted the update-vislayer-models branch February 4, 2023 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-anywhere visualizations Issues and PRs related to visualizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants