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

[Lens] Default timefield as a missing field is causing an esaggs error #84058

Closed
mbondyra opened this issue Nov 23, 2020 · 10 comments · Fixed by #85754
Closed

[Lens] Default timefield as a missing field is causing an esaggs error #84058

mbondyra opened this issue Nov 23, 2020 · 10 comments · Fixed by #85754
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@mbondyra
Copy link
Contributor

  1. Type this into devtools to create a simple index.
DELETE simpletimeindex3

POST simpletimeindex3/_doc
{
  "ts1": "2020-09-30",
  "val": 1
}

POST simpletimeindex3/_doc
{
  "ts1": "2020-10-01",
  "val": 1
}
  1. Create index pattern with timefield set to ts1.
  2. Go to lens and create any visualization. Save.
  3. Go back to dev tools and type this - it will remove your index and create almost identical one, but with different time index name.
DELETE simpletimeindex3

POST simpletimeindex3/_doc
{
  "ts2": "2020-09-30",
  "val": 1
}

POST simpletimeindex3/_doc
{
  "ts2": "2020-10-01",
  "val": 1
}
  1. Go to index patterns settings -> find the created index pattern and refresh the list of fields.
  2. Go to lens. The visualization is showing not very readable error and there's no way to fix it.
    image

Expected behavior: I am not sure if we should 'recover' the visualization in this case by changing the timefield. Maybe we should only improve messaging? Removing all the columns help, but once the user wants to build date histogram on the proper timefield, the error surfaces again.

@mbondyra mbondyra added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Nov 23, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@mbondyra mbondyra changed the title [Lens] Default timefield as a missing field is causing a esaggs warning [Lens] Default timefield as a missing field is causing an esaggs warning Nov 23, 2020
@mbondyra mbondyra changed the title [Lens] Default timefield as a missing field is causing an esaggs warning [Lens] Default timefield as a missing field is causing an esaggs error Nov 23, 2020
@dej611
Copy link
Contributor

dej611 commented Nov 23, 2020

I'd say that the behaviour here should match the #81439 one, as when there's a missing field.
But I see the peculiarity of the ts1 set as default timeField.
Perhaps the original check could be extended, wdyt @flash1293 ?

@flash1293
Copy link
Contributor

@mbondyra So this error happens because the index pattern still has the default time field set to a non-existent field? If yes, then IMHO this is an app-services bug and should be handled on the index pattern level by resetting the default time field.

@flash1293 flash1293 added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:AppServices labels Nov 23, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@flash1293
Copy link
Contributor

@mattkime This seems to be a consistency problem of index patterns - if a field disappears from the field list, it shouldn't be possible to have it listed as default time field anymore. Maybe this is something to take care of in your current PR removing the cached field list?

@mbondyra
Copy link
Contributor Author

mbondyra commented Nov 25, 2020

@mattkime @flash1293 I've checked how is visualize handling it and it's basically the same problem - visualization is not showing anything, the timefield cannot be changed and there's an error in the console (I changed ts to ts1, but you cannot change it in the select list). A user is not able to fix the visualization.

image

@mattkime
Copy link
Contributor

mattkime commented Dec 1, 2020

@flash1293 I don't think it make sense to change the index pattern timestamp field based on which fields are available. The fields can change. Just as the timestamp field can be missing from docs, it can also be restored. Further, there's not really a way of resolving the issue of a missing timestamp field. We can return undefined which is what we currently do.

That said, the error could certainly be handled better.

It seems to me that it might be handled on two levels - a universal error message that states the problem and a use case specific graceful handling of the problem.

I do like the idea of being able to fix the problem directly from lens. This might help get us there - #67711

@flash1293
Copy link
Contributor

flash1293 commented Dec 1, 2020

@mattkime Maybe I should be more specific - I don't expect the saved object to be updated or anything and resolving this by selecting another time field or similar. The current problem @mbondyra reported is caused by this line:

return agg.getIndexPattern().timeFieldName;

Further, there's not really a way of resolving the issue of a missing timestamp field. We can return undefined which is what we currently do.

This is not the case in all situations, at least not in the one linked above. It is referring to the non-existent field which is causing this exception within the esaggs function even if the Lens config is never referencing the broken field.

My proposal is: If the default time field is not part of the fields, behave as if there is no default time field - we don't have to error out in this case.

I do like the idea of being able to fix the problem directly from lens. This might help get us there - #67711

It would be cool to edit the index pattern from within Lens, but this is too far out IMHO for fixing this bug.

@mattkime
Copy link
Contributor

mattkime commented Dec 2, 2020

@flash1293

If the default time field is not part of the fields, behave as if there is no default time field - we don't have to error out in this case.

Its still not clear to me how this relates to the index pattern code. Doesn't it relate to the code in date_histogram.ts?

Perhaps you're suggesting that when an index pattern doesn't have a field for its set timeFieldName it should return undefined for timeFieldName. I'm hesitant to do that based on what I see here. I'd prefer a simple getter remain a simple getter. Similar behavior is available through getTimeField()?.name.

@flash1293
Copy link
Contributor

Its still not clear to me how this relates to the index pattern code

Sorry, I wrapped up too much into a single paragraph. Yeah, I was referring to return undefined for the timeFieldName if it's not available so a consumer can rely on indexPattern.timeFieldName !== undefined implying indexPattern.getTimeField() !== undefined (as there are at least two places in the code already assuming that, one in Lens and the other one in the date_histogram agg - didn't check Discover but it's using timeFieldName as well so it's likely there is unexpected behavior hidden in there as well).

I'm fine with fixing this in the consumers by not assuming that implication and explicitly checking whether the referenced field exists, but it seems worth documenting it explicitly as part of the API as it's definitely easy to miss.

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 Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants