-
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
[Lens] Pass used histogram interval to chart #91370
[Lens] Pass used histogram interval to chart #91370
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
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.
Tested and this works as expected!
serialize(val, aggConfig) { | ||
// store actually used auto interval in serialized agg config to be able to read it from the result data table meta information | ||
const autoBounds = aggConfig?.getAutoBounds(); | ||
if (val === autoInterval && aggConfig && autoBounds) { |
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.
This logic is duplicated with the write
and serialize
functions. I can't think of a better way, but maybe you can?
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.
this seems very different to how we handle this for date_histogram:
we don't put this is serialized agg config, but rather just on the datatable column meta information. We also use its own property to store that, instead of merging it into existing property.
could we do the same thing we do for date_histogram for histgram as well ? or change how we do date_histogram ?
in either case i would prefer to have a separate property for storing this, which can also be achieved using the aggconfigs serialization.
intervalBase: aggConfig.params.intervalBase, | ||
esTypes: aggConfig.params.field?.spec?.esTypes || [], | ||
}); | ||
return buildSerializedAutoInterval(usedInterval); |
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 could create a new property on this aggConfig calculatedInterval
and make its serialize method:
serialize: (val, aggConfig) => {
const autoBounds = aggConfig?.getAutoBounds();
if (aggConfig && autoBounds && aggConfig.params.interval === autoInterval ) {
const usedInterval = calculateHistogramInterval({
values: autoBounds,
interval: aggConfig.params.interval,
maxBucketsUiSettings: getConfig(UI_SETTINGS.HISTOGRAM_MAX_BARS),
maxBucketsUserInput: aggConfig.params.maxBars,
intervalBase: aggConfig.params.intervalBase,
esTypes: aggConfig.params.field?.spec?.esTypes || [],
});
return usedInterval;
}
return aggConfig.params.interval;
}
we don't need any deserialize method for it, write should be _.noop
@ppisljar The difference to date histogram is that the real interval is only known during data fetching (for date histogram we can derive it statically based on the current time range, but for histogram auto interval we need the extra request to fetch min and max). I really like your idea of introducing a hidden "param" which is just used for serialization, that's a much cleaner implementation. I will change the implementation on this PR. |
but we could do date_histogram in the same way as well right, thus eliminate the need of tabify having special knowledge about it ? |
@ppisljar Yes, that's a good point. I can do the refactoring for this on this PR as well. |
@ppisljar I refactored the number histogram interval as discussed, could you take a look? I tried to do the date histogram in the same way, but it's a little bigger in scope than I thought, I will create a separate tech debt issue for this. |
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.
code LGTM
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* master: (111 commits) [Logs UI] Replace dependencies in the infra bundle (elastic#91503) [Search Source] Do not request unmapped fields if source filters are provided (elastic#91921) [APM] Kql Search Bar suggests values outside the selected time range (elastic#91918) Refactored component edit policy tests into separate folders and using client integration testing setup (elastic#91657) [Fleet] Don't error on missing package_assets value (elastic#91744) [Lens] Pass used histogram interval to chart (elastic#91370) [Indexpattern management] Use indexPatterns Service instead of savedObjects client (elastic#91839) [Security Solutions] Fixes Cypress tests for indicator match by making the selectors more specific (elastic#91947) [CI] backportrc can skip CI (elastic#91886) Revert "[SOM] fix flaky suites (elastic#91809)" [Fleet] Install Elastic Agent integration by default during setup (elastic#91676) [Fleet] Silently swallow 404 errors when deleting ingest pipelines (elastic#91778) [data.search] Use incrementCounter for search telemetry (elastic#91230) [Fleet] Bootstrap functional test suite (elastic#91898) [Alerts][Docs] Added API documentation for alerts plugin (elastic#91067) Use correct environment in anomaly detection setup link (elastic#91877) [FTSR] Convert to tasks and add jest/api integration suites (elastic#91770) [CI] Build and publish storybooks (elastic#87701) docs: add PHP agent info to docs (elastic#91773) [DOCS] Adds and updates Visualization advanced settings (elastic#91904) ...
Closes #85924
We previously fixed a similar but for date histograms. In order to draw bars of the correct with, the visualization has to know the underlying interval. elastic-charts tries to infer it from the data (e.g. if one data point starts at 20 and the next at 30, it assumes an interval of 10), but this strategy doesn't work for sparse data. This PR makes it explicit by storing the used interval in the serialized agg config and providing a helper to read the interval from the column.
To test, go to Lens with logs sample data and configure a "bytes" histogram with count of records, then pick a small time range (e.g. 30mins). There will only be a few data points, so the chart can't infer the interval from the data.
This PR:
master:
This will allow us to fix this bug #91481 as well because this way we can pass the information about the actually used interval to the filter building logic.