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

Move Fields from pacakge libbeat/common to libbeat/mapping #11198

Merged
merged 5 commits into from
Mar 14, 2019

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Mar 11, 2019

The structures Field and Fields are moved from common to the package mapping.
This is a step towards moving things from the abused package common.
I would like to move fields and other asset related functionality to this package in the future.

@kvch kvch force-pushed the refactoring-libbbeat-moving-fields branch from 7988670 to 9e75599 Compare March 11, 2019 20:28
@kvch kvch marked this pull request as ready for review March 11, 2019 20:29
@kvch kvch requested a review from a team as a code owner March 11, 2019 20:29
@kvch kvch changed the title Loading external fields.yml files Move Fields from pacakge common to asset Mar 11, 2019
@kvch kvch mentioned this pull request Mar 11, 2019
2 tasks
@kvch kvch requested review from urso and ph and removed request for a team March 11, 2019 20:32
@kvch kvch removed the enhancement label Mar 12, 2019
@kvch kvch requested a review from a team as a code owner March 12, 2019 13:27
@kvch kvch removed the request for review from a team March 12, 2019 13:29
@ruflin
Copy link
Member

ruflin commented Mar 12, 2019

+1 on moving field out, but I don't think there is a direct relation between asset and field. Field is not even used in the asset package as far as I remember. I would like to keep the asset package reserved for the registry and asset generation. What about moving field to it's own package, for example common/field?

@urso
Copy link

urso commented Mar 12, 2019

Maybe time to define a project name for fields.yml.

I'm no fan of the common package alltogether. Same goes for sub-packages named after common/<x>.

How about renaming the Fields type to Document, or Schema while we are at it? Such that we have field.Schema or field.Document, assuming we named the package/project field.

Alternative package names:

  • fields
  • schema
  • mapping
  • diag(ramming)
  • (evt/event-)model

@kvch
Copy link
Contributor Author

kvch commented Mar 12, 2019

@ruflin I have moved the collection of fields be it in the form of an encoded string or yaml in a draft PR where I implement supporting additional fields.yml files. My reasoning was that every data/resource is an asset which is required to generate a template. As Field/Fields is a data structure required for that process, it seems like the perfect place for this. I also tried to avoid a Golang anti-pattern of creating many small packages. But I am fine with keeping this small. I think we need to define the term asset to make a proper decision.

@urso Great idea! I like the name Schema as these fields define the composition of a template and provide additional metadata (e.g. documentation).Maybe mapping.Schema would be the most appropriate as these fields supposed to describe a mapping of an index. Hm... Sounds a bit weird. Or mapping.Document as it stores a mapping of a document? I need a bit more time to think.

@ruflin
Copy link
Member

ruflin commented Mar 13, 2019

The asset package is not only intended to package fields but the goal was that it can handle anything we want as asset in the golang code. This could also be a config file or an ingest pipeline or other things. That is why I would prefer not to have it in this package.

If I understand it correctly, this package change is needed for a follow up PR? To not block this, I suggest we move it for now to its own package and follow up with a renaming discussion. For schema keep in mind, we have an other schema package ...

@kvch
Copy link
Contributor Author

kvch commented Mar 13, 2019

@ruflin @urso I moved Field and Fields from asset to mapping. I think this package name is the most suitable because these fields describe an ES mapping.

@kvch kvch changed the title Move Fields from pacakge common to asset Move Fields from pacakge common to libbeat/mapping Mar 13, 2019
@kvch kvch changed the title Move Fields from pacakge common to libbeat/mapping Move Fields from pacakge libbeat/common to libbeat/mapping Mar 13, 2019
@kvch
Copy link
Contributor Author

kvch commented Mar 13, 2019

Failing tests are unrelated to this change.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM, don't have strong opinions about the naming of the package.

@elastic/apm-server This change might affect apm-server too?

@ruflin
Copy link
Member

ruflin commented Mar 13, 2019

@kvch Any idea what happened with the windows tests in https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-windows/5947/beat=filebeat,label=windows/ Never saw them failing at once all together. Will rerun CI to be sure it's not related.

@ruflin
Copy link
Member

ruflin commented Mar 13, 2019

jenkins, test this

@kvch
Copy link
Contributor Author

kvch commented Mar 13, 2019

@ruflin i have reopened the tickets of those flaky tests

@kvch kvch merged commit e5ac246 into elastic:master Mar 14, 2019
graphaelli added a commit to simitt/apm-server that referenced this pull request Mar 22, 2019
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.

4 participants