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

[Data Usage] updates to metrics api #195640

Merged
merged 7 commits into from
Oct 10, 2024

Conversation

neptunian
Copy link
Contributor

@neptunian neptunian commented Oct 9, 2024

  • validates autoOps response data using mock data and new type
  • processes autoOps data to return an object of {x,y} values from our API instead of array of [timestamp, value]. updates UI accordingly
  • requires data_streams from the /metrics api
  • changes the /metrics route from GET to POST, better suited for more complex and larger payloads

@neptunian neptunian added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 9, 2024
@neptunian neptunian requested review from a team as code owners October 9, 2024 16:05
@neptunian neptunian enabled auto-merge (squash) October 9, 2024 17:23
@neptunian neptunian changed the title [Data Usage] process autoops mock data [Data Usage] updates to metrics api Oct 9, 2024
expand_wildcards: 'all',
});

const hasDataStreams = dataStreamsResponse.length > 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

esClient.indices.getDataStream will throw an error if a requested data stream is not found. since data streams are required, the logic below would not be met.

Comment on lines +47 to +50
dataStreams: [
'.alerts-ml.anomaly-detection-health.alerts-default',
'.alerts-stack.alerts-default',
],
Copy link
Member

Choose a reason for hiding this comment

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

Q: With respect to UX, should we use all the datastreams from the API here and have them all selected when we land on the page even if the list is very long, say 100 or more?

Copy link
Contributor Author

@neptunian neptunian Oct 10, 2024

Choose a reason for hiding this comment

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

Probably a question for a PM. Personally, I would say the "top" 10 would suffice. Otherwise we are potentially making the initial experience very slow unnecessarily.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that is my concern. Also since we're pulling the datastreams list from another API (/_metering/stats) that doesn't allow for paging, I'm not sure how we're going to handle cases when users have a huge list of data streams.

Copy link
Contributor Author

@neptunian neptunian Oct 10, 2024

Choose a reason for hiding this comment

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

At the moment I think the plan is to not handle it and just display them all in the dropdown but only select the largest by storage size of 10. The dropdown with all the streams will ideally scroll and not go off the page.

}),
])
),
dataStreams: schema.arrayOf(schema.string(), {
Copy link
Member

Choose a reason for hiding this comment

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

This schema has to be a oneOf(string, array). It should work with api/path?dataStreams=xyz and with api/path?dataStreams=xyz&dataStreams=pqr. The first one is a string and the second one is an array. So you can use the previous schema def without the maybe wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I think in my head I was thinking this should be a POST request and we could simplify it and metricTypes to be arrays only. Given they can pass in as many data streams as they want I think we should probably switch to a POST as I can see this URL getting very large. wdyt?

Comment on lines +47 to +51
if (trimmedValues.some((v) => !v.length)) {
return '[metricTypes] list cannot contain empty values';
} else if (trimmedValues.some((v) => !isValidMetricType(v))) {
return `[metricTypes] must be one of ${METRIC_TYPE_VALUES.join(', ')}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should also validate a string input instead of an array then the error should say that it expects and array and not a string. could not parse array value from json input is not valuable info to fix the request input, IMO.

dataStreams: schema.arrayOf(schema.string(), {
minSize: 1,
validate: (values) => {
if (values.map((v) => v.trim()).some((v) => !v.length)) {
Copy link
Member

Choose a reason for hiding this comment

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

same validation here for a string intput instead of array

@@ -42,37 +41,37 @@ export const DataUsage = () => {
setUrlDateRangeFilter,
} = useDataUsageMetricsUrlParams();

const [queryParams, setQueryParams] = useState<UsageMetricsRequestSchemaQueryParams>({
const [metricsFilters, setMetricsFilters] = useState<UsageMetricsRequestSchemaQueryParams>({
Copy link
Member

Choose a reason for hiding this comment

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

note: This file is renamed in my PR. you can leave it here as it is and I'll resolve conflicts in my PR.

@neptunian neptunian merged commit 3ec1908 into elastic:main Oct 10, 2024
21 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11281103237

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataUsage 229.9KB 229.7KB -210.0B

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 10, 2024
- validates autoOps response data using mock data and new type
- processes autoOps data to return an object of {x,y} values from our
API instead of array of [timestamp, value]. updates UI accordingly

(cherry picked from commit 3ec1908)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 10, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Data Usage] process autoops mock data
(#195640)](#195640)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sandra
G","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-10T19:59:48Z","message":"[Data
Usage] process autoops mock data (#195640)\n\n- validates autoOps
response data using mock data and new type\r\n- processes autoOps data
to return an object of {x,y} values from our\r\nAPI instead of array of
[timestamp, value]. updates UI
accordingly","sha":"3ec190823fa39520dc50f5c4631eb81cd223ed3e","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor"],"title":"[Data
Usage] updates to metrics
api","number":195640,"url":"https://github.com/elastic/kibana/pull/195640","mergeCommit":{"message":"[Data
Usage] process autoops mock data (#195640)\n\n- validates autoOps
response data using mock data and new type\r\n- processes autoOps data
to return an object of {x,y} values from our\r\nAPI instead of array of
[timestamp, value]. updates UI
accordingly","sha":"3ec190823fa39520dc50f5c4631eb81cd223ed3e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195640","number":195640,"mergeCommit":{"message":"[Data
Usage] process autoops mock data (#195640)\n\n- validates autoOps
response data using mock data and new type\r\n- processes autoOps data
to return an object of {x,y} values from our\r\nAPI instead of array of
[timestamp, value]. updates UI
accordingly","sha":"3ec190823fa39520dc50f5c4631eb81cd223ed3e"}}]}]
BACKPORT-->

Co-authored-by: Sandra G <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants