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.

Sign up for GitHub

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

Jump to bottom

[System][Core] Add dimensions for the core data_stream #6454

Merged
merged 11 commits into from
Jun 30, 2023
Merged

[System][Core] Add dimensions for the core data_stream #6454

merged 11 commits into from
Jun 30, 2023

Conversation

tetianakravchenko
Copy link
Contributor

@tetianakravchenko tetianakravchenko commented Jun 2, 2023 •
edited
Loading

What does this PR do?

  • add dimensions metadata to the fields
  • clean up duplicated fields
  • add missing fields, that were added recently in beats

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@tetianakravchenko
add dimension for the core data_stream
7bda7f4 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@elasticmachine
Copy link

elasticmachine commented Jun 2, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-06-30T06:20:35.833+0000

  • Duration: 15 min 44 sec

Test stats 🧪

Test Results
Failed 0
Passed 145
Skipped 0
Total 145

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Jun 2, 2023 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (4/4) 💚
Classes 100.0% (4/4) 💚
Methods 60.759% (48/79) ❕
Lines 99.536% (2790/2803) ❕
Conditionals 100.0% (0/0) 💚

This was referenced Jun 5, 2023
Closed
Closed
@tetianakravchenko
merge, fix conflicts
954433d 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko
update version
6051ad9 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko
change the type of the core.id field to long; enable tsdb on this dat…
55d0bb4 
…a_stream

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko tetianakravchenko marked this pull request as ready for review June 6, 2023 09:53
@tetianakravchenko tetianakravchenko requested a review from a team as a code owner June 6, 2023 09:53
tetianakravchenko
tetianakravchenko commented Jun 6, 2023
View reviewed changes
packages/system/data_stream/core/fields/fields.yml Outdated
@@ -2,7 +2,8 @@
type: group
fields:
- name: id
type: keyword
type: long
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this field is of type long in mericbeat - https://github.com/elastic/beats/blob/main/metricbeat/module/system/core/_meta/fields.yml#L1-L10

enablement of TSDB does not work in case of using keyword due to this issue - elastic/elasticsearch#96552

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: after installing locally this version with changed mapping/added dimensions/enabled tsdb:
In Data View:
Screenshot 2023-06-06 at 11 29 29

Amount of document before and after installing new version - the same

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this change be a breaking change?

An alternative option, having minimal impact which can be considered is to introduce a new field , say core_long_id or similar having type long.

Copy link
Contributor Author

@tetianakravchenko tetianakravchenko Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could this change be a breaking change?

I don't think that this should be considered as breaking change, @lalit-satapathy please correct me if I am wrong

An alternative option, having minimal impact which can be considered is to introduce a new field , say core_long_id or similar having type long.

I think it is better to wait than this fix - elastic/elasticsearch#96552 than introducing, then deprecating and removing a redundant field, used just to set a dimension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: as agreed in elastic/elasticsearch#96552 (comment), by d4910e7 commit:

  • do not enable tsdb on this data_stream
  • revert core.id type back to keyword

@agithomas could you please review this PR again?

@tetianakravchenko
add missing fields
699482b 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko
revert enablement of tsdb on the core dat_stream; revert system.core.…
d4910e7 
…id type to keyword

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko tetianakravchenko requested a review from a team as a code owner June 21, 2023 12:07
@tetianakravchenko
run elastic-package build
9d368ae 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko
merge main branch; fix conflicts
4984b1e 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko
Merge branch 'master' into system-core-dimensions
610916d
@tetianakravchenko
Copy link
Contributor Author

/test

@tetianakravchenko
remove new fields
d477348 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko
build package to update documentation
e2b5cd0 
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
agithomas
agithomas approved these changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM!

@tetianakravchenko tetianakravchenko merged commit 65d849f into elastic:main Jun 30, 2023
@tetianakravchenko tetianakravchenko deleted the system-core-dimensions branch June 30, 2023 06:38
@elasticmachine
Copy link

Package system - 1.34.1 containing this change is available at https://epr.elastic.co/search?package=system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agithomas agithomas agithomas approved these changes

@rdner rdner Awaiting requested review from rdner rdner was automatically assigned from elastic/elastic-agent-data-plane

@faec faec Awaiting requested review from faec faec was automatically assigned from elastic/elastic-agent-data-plane

@fearful-symmetry fearful-symmetry Awaiting requested review from fearful-symmetry

@leehinman leehinman Awaiting requested review from leehinman

Assignees
No one assigned
Labels
Integration:system System
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tetianakravchenko @elasticmachine @agithomas @andrewkroh