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

Elasticsearch: Create Raw Data metric to render raw JSON docs in columns in the new table panel #26233

Merged
merged 18 commits into from
Jul 15, 2020

Conversation

ivanahuckova
Copy link
Member

@ivanahuckova ivanahuckova commented Jul 10, 2020

What this PR does / why we need it:

Follows up on #24521. To be backwards compatible, I have created Raw Document v2 metric to process results as DataFrames and not SeriesList.

Blocker:
When switching between the Raw Document and Raw Document v2 table does not update as new request is not sent (because the request is the same).

Which issue(s) this PR fixes:

Fixes #24163

@ivanahuckova ivanahuckova requested review from a team and davkal and removed request for a team and davkal July 10, 2020 11:49
@ivanahuckova
Copy link
Member Author

@marefr @torkelo Could you please check this out and share your thoughts? I have followed up on #24521 and created a new metric as suggested.

Also, do you have any suggestions on how to fix the blocker - when switching between the Raw Document and Raw Document v2 table does not update as new request is not sent (because the request is the same).

@torkelo
Copy link
Member

torkelo commented Jul 10, 2020

not sure I understand why a new query is not issued, you will need to trigger a new query for the result processing to be re-executed

Raw Document v2 is not great. Maybe

  • Document fields
  • Raw data

and then change text label (not value) for the old Raw document to Raw document (legacy).

@ivanahuckova
Copy link
Member Author

not sure I understand why a new query is not issued, you will need to trigger a new query for the result processing to be re-executed

Figured that out in query_ctrl we are checking if queries are the same. I have added exception for Raw Data and Raw Doocument.

Raw Document v2 is not great. Maybe
Document fields
Raw data
and then change text label (not value) for the old Raw document to Raw document (legacy).

Changed to Raw Data, as I feel it is more clear and for intuitive for users based on Raw Document naming. Also they are right next to each other, so users can easily try it out, when they see Raw document (legacy).

@ivanahuckova ivanahuckova changed the title WIP - Elasticsearch: Fixes raw document query render in table panel Elasticsearch: Fixes raw document query render in table panel Jul 10, 2020
@ivanahuckova ivanahuckova self-assigned this Jul 13, 2020
@ivanahuckova ivanahuckova changed the title Elasticsearch: Fixes raw document query render in table panel Elasticsearch: Create Raw Doc metric to render raw JSON docs in columns in the new table panel Jul 13, 2020
@ivanahuckova ivanahuckova added this to the 7.1 milestone Jul 13, 2020
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Have tested this. In general I wonder whether introducing a different Metric is the way to go. Worth consider having an option above or below Size when Raw Document is selected. Maybe a switch/boolean for legacy format that's true if this property is null/undefined and when adding a new query and selecting Raw Document this gets set to false per default. Doing this gives us possibility to also add some tooltip/description of what it means. Thought?

Found some bugs.

Using Raw Data I cannot edit size option:
image

Comparing with old table panel and Raw Document query it doesn't include _source as a column, but using Raw Data does which feels wrong:
image

@ivanahuckova
Copy link
Member Author

ivanahuckova commented Jul 13, 2020

Using Raw Data I cannot edit size option

✅ Nice catch! Fixed in 2096cd0

Comparing with old table panel and Raw Document query it doesn't include _source as a column, but using Raw Data does which feels wrong

✅ Removed in 611b692

Have tested this. In general I wonder whether introducing a different Metric is the way to go. Worth consider having an option above or below Size when Raw Document is selected.

Well yeah, that is a really good question and I think I might like the solution that you have just proposed slightly better. I have created a new metric as both you and @torkelo have suggested it in #24521 (comment) and #24163 (comment), so I thought that there was a consensus that this would be the best option. But I try to create another PR with the other solution and we can just choose whichever we like more. But we should make sure that this fix goes to 7.1 as it was promised. 🙂

@ivanahuckova
Copy link
Member Author

ivanahuckova commented Jul 14, 2020

I have created a new PR for the "switch-idea" #26323.

marefr
marefr previously requested changes Jul 15, 2020
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Logs in Explore seems to be broken (works in master):
image

If this was caused by removing _source column I'm fine with skipping that change if it resolves explore/logs problem

@ivanahuckova ivanahuckova modified the milestones: 7.1, 7.2 Jul 15, 2020
@ivanahuckova
Copy link
Member Author

@marefr fixed in 3687e94

@ivanahuckova
Copy link
Member Author

Kapture 2020-07-15 at 11 37 03

@ivanahuckova ivanahuckova modified the milestones: 7.2, 7.1 Jul 15, 2020
@ivanahuckova ivanahuckova requested a review from marefr July 15, 2020 09:38
@@ -51,9 +51,15 @@ export class ElasticQueryCtrl extends QueryCtrl {
}

queryUpdated() {
// As Raw Data and Raw Document have the same request, we need to run refresh if they are updated
const isPossiblyRawDataSwitch = this.target.metrics.some(
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get this logic here. We are checking only new ones so we do not actually compare to old one. Probably works but seems unintuitive here. Can we just use something similar to what we already have like this.oldTargetMetric and then compare it to new one?

Probably a bit nitpicky, can be skipped if this should be time consuming, but I think it would make more clear what we are actually trying to figure out here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will create another PR for this.

@ivanahuckova ivanahuckova dismissed marefr’s stale review July 15, 2020 13:20

All requested changes by Marcus are implemented. PR was reviewed by Andrej as Marcus is busy with other bug that directly affects 7.1 release.

@ivanahuckova ivanahuckova merged commit 3fd8104 into master Jul 15, 2020
@ivanahuckova ivanahuckova deleted the ivana/elastic-table branch July 15, 2020 13:20
dprokop pushed a commit that referenced this pull request Jul 16, 2020
…ns in the new table panel (#26233)

* test

* WIP: Create v2 version

* Update tests, remove conosole logs, refactor

* Remove incorrect types

* Update type

* Rename legacy and new metrics

* Update

* Run request when Raw Data tto Raw Document switch

* Fix size updating

* Remove _source field from table results as we are showing each source field as column

* Remove _source just for metrics, not logs

* Revert "Remove _source just for metrics, not logs"

This reverts commit 611b692.

* Revert "Remove _source field from table results as we are showing each source field as column"

This reverts commit 31a9d5f.

* Add vis preference for logs

* Update visualisation to logs

* Revert "Revert "Remove _source just for metrics""

This reverts commit a102ab2.

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
(cherry picked from commit 3fd8104)
@dprokop dprokop mentioned this pull request Jul 16, 2020
dprokop pushed a commit that referenced this pull request Jul 16, 2020
…ns in the new table panel (#26233)

* test

* WIP: Create v2 version

* Update tests, remove conosole logs, refactor

* Remove incorrect types

* Update type

* Rename legacy and new metrics

* Update

* Run request when Raw Data tto Raw Document switch

* Fix size updating

* Remove _source field from table results as we are showing each source field as column

* Remove _source just for metrics, not logs

* Revert "Remove _source just for metrics, not logs"

This reverts commit 611b692.

* Revert "Remove _source field from table results as we are showing each source field as column"

This reverts commit 31a9d5f.

* Add vis preference for logs

* Update visualisation to logs

* Revert "Revert "Remove _source just for metrics""

This reverts commit a102ab2.

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
(cherry picked from commit 3fd8104)
@ivanahuckova ivanahuckova changed the title Elasticsearch: Create Raw Doc metric to render raw JSON docs in columns in the new table panel Elasticsearch: Create Raw Data metric to render raw JSON docs in columns in the new table panel Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch: Raw document query render raw JSON documents in each row in the new table panel
5 participants