-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security solution] Implement dashboard to track Gen AI Token Usage #159075
Conversation
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.
Looking good! Made a few requests to changes to the logging and some tests.
x-pack/plugins/stack_connectors/server/connector_types/gen_ai/create_dashboard.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/stack_connectors/server/connector_types/gen_ai/dashboard.ts
Show resolved
Hide resolved
body: { | ||
index: [ | ||
{ | ||
names: ['*.kibana-event-log-*'], |
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.
We definitely should not use *
at the beginning, Could have been done because the new index names of the datastream-y event log are: .ds-.kibana-event-log-8.9.0-2023.06.12-000001
- but I just tried, and this API seems to do the right thing when using .kibana-event-log-*
- presumably checking the data stream.
.../alerting_api_integration/security_and_spaces/group2/tests/actions/connector_types/gen_ai.ts
Show resolved
Hide resolved
body: { | ||
index: [ | ||
{ | ||
names: ['*.kibana-event-log-*'], |
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.
Kind of a fuzzy line - I was also a little surprised this was a subcommand and not an internal route. Technically, I don't think there's much of a difference. To some extent, subcommands are "managed" in the code a little better than routes, that sometimes end up in odd places. And less ceremony, and chances that we got the auth wrong in the route, etc.
I think the thing that separates this from things like Jira, is that this subcommand doesn't really do anything with the connector. It literally could be a stand-alone route, anywhere. It doesn't NEED to be in the connector.
The other aspect is backwards compatibility. We consider connectors to be API, so we would have to support this subcommand forever. The chances that someone would use this externally seem low, but customers are crafty, and someone doing infrastructure-as-code might realize what's going on, and bake this into some scripts of theirs. Which would then break their scripts when we remove it. The chances of that happening with an internal HTTP API seems smaller.
Risk seems very low with this one, especially if this is temporary. If we don't think it's temporary, converting to an internal route after FF next week is a possibility as well.
const setUrl = useCallback( | ||
(dashboardId: string) => { | ||
const url = dashboard?.locator?.getRedirectUrl({ | ||
query: { |
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.
A nice enhancement for later would be looking into making this one of the "pilled" filter thingees, instead of the query. That would leave the query empty for customers to do further querying without having to worry about the connector match clause. Not actually sure that's possible, but think it is.
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.
Interesting, i will research that further
@pmuellr re: #159075 (comment) Yeah I felt weird about that too, but my original approach I had used a index method I found in being used to match indices in I will remove it on my code for sure |
Ah! Ya, that code is for users searching through their own indices. I think the feeling was a the time, if they typed If you're not all finished, wanna add a comment yourself? Else I will do a quick one. Suggestion: // prepend and append index search requests with `*` to match the given text in middle of index names |
@elasticmachine merge upstream |
@@ -41,7 +41,7 @@ export class ObjectRemover { | |||
`${getUrlPrefix(spaceId)}/${isInternal ? 'internal' : 'api'}/${plugin}/${type}/${id}` | |||
) | |||
.set('kbn-xsrf', 'foo') | |||
.expect(204); | |||
.expect(plugin === 'saved_objects' ? 200 : 204); |
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.
I'm deleting a saved object, and the successful response for this api is 200
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.
LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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.
Nicely done! Thank you Steph! LGTM!
Summary
Resolves https://github.com/elastic/security-team/issues/6712
getDashboard
. The subaction checks if a dashbord with a given id exists, and if not creates the dashboard. Returns{ exists: true }
upon successful retreival/creation{ exists: false }
if the user does not have permissions.getDashboard
subaction returns{ exists: true }
andisEdit
is true, the connector form displays a link to the dashboard filtered by the connector idType confusion
It seems the
stack_connectors
uses thetriggers_actions_ui
CONNECTORS_PLUGIN_ID
which has its ownTriggersAndActionsUiServices
type. However, when I importuseKibana
it is tied to the type forPLUGIN_ID
. That is why I extended both versions of the app to includedashboard
. Please let me know if you think I should instead maybe export auseKibana
that is specifically typed to the connector?To test
Credentials for OpenAI and Azure OpenAI: https://p.elstc.co/paste/+6nHnsHt#WJOOGIkVoJjTq7zPLP77BzSG1AUxNFUDvcnSfLPtO7m
Testing permissions
Update 6/12/23: Updated to selectively show the link to the dashboard based on user permissions
Update 6/13/23: Added a markdown text block to explain dashboard permissions, content to be updated with a doc link