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

Stage 2 changes for RFC 0009 - data_stream fields #1307

Merged
merged 8 commits into from
Apr 19, 2021

Conversation

ebeahan
Copy link
Member

@ebeahan ebeahan commented Mar 18, 2021

Adding the data_stream fields from RFC 0009 (#1367 ) to the schema as beta fields.

ECS docs preview

@ebeahan
Copy link
Member Author

ebeahan commented Mar 18, 2021

@elasticmachine run elasticsearch-ci/docs

@ebeahan ebeahan requested review from ruflin, djptek and kgeller March 18, 2021 20:21
@ruflin ruflin requested a review from jpountz March 22, 2021 11:52
@ruflin
Copy link
Contributor

ruflin commented Mar 22, 2021

I'm pretty excited to see this moving forward. @jpountz You were previously concerned about already moving this forward. You think we should still hold back on this or ok to get this shipped and improve in the future on top of it?

kgeller
kgeller previously approved these changes Mar 22, 2021
@ebeahan ebeahan added the 1.10.0 label Apr 6, 2021
@ebeahan
Copy link
Member Author

ebeahan commented Apr 6, 2021

@ruflin Are we still waiting for any feedback before merging?

@ruflin
Copy link
Contributor

ruflin commented Apr 7, 2021

Good to go on my end but I would like to have a 👍 or 👎 from @jpountz as he had initially some concerns.

@ebeahan ebeahan force-pushed the rfc/0009/apply-stage-3-changes branch from 8d3c5ff to e28c4d3 Compare April 13, 2021 17:32
@ebeahan
Copy link
Member Author

ebeahan commented Apr 13, 2021

Good to go on my end but I would like to have a 👍 or 👎 from @jpountz as he had initially some concerns.

Ok. The implementation here is based on the design decisions agreed upon in the RFC.

If there are still concerns, maybe we reconsider if the RFC is finished and reopen the discussion?

@jpountz
Copy link
Contributor

jpountz commented Apr 14, 2021

The thing that's unclear to me is what GA means for ECS. I'm fine with GA-ing field names and what they mean. However there's an active discussion about changing the way that these fields are mapped to extract them dynamically from the name of the data stream rather than using constant_keyword and requiring data shippers to send these values as part of documents. Should we refrain from making these fields GA until this discussion settles?

@ebeahan
Copy link
Member Author

ebeahan commented Apr 14, 2021

The thing that's unclear to me is what GA means for ECS. I'm fine with GA-ing field names and what they mean.

Once fields are considered GA, apps/content that produce or consume ECS data can rely on those fields to remain stable with breaking changes only at major versions. ECS data producers can confidently populate, and consumers can operate against those fields.

However there's an active discussion about changing how these fields are mapped to extract them dynamically from the name of the data stream rather than using constant_keyword and requiring data shippers to send these values as part of documents. Should we refrain from making these fields GA until this discussion settles?

I believe the motivation for adding these fields into ECS is their importance in the recommended indexing strategy. If there's a change in direction that'll make those fields obsolete soon, I think we should hold off on GA-ing the data_stream.* fields.

Another option is to add the data_stream.* fields but mark them as beta in the docs (and roll back the RFC to stage 2).

@jpountz
Copy link
Contributor

jpountz commented Apr 15, 2021

My perspective is that these fields will stay and that we won't break data consumers (like Kibana), however the changes we are considering might break data producers (like Agent). Adding them as beta first sounds good to me.

@ebeahan
Copy link
Member Author

ebeahan commented Apr 15, 2021

Adding them as beta first sounds good to me.

@ruflin thoughts on adding these data_stream fields as beta to give us more flexibility while discussion continues around a potentially different, future approach?

@ruflin
Copy link
Contributor

ruflin commented Apr 18, 2021

I don't think we will have the option to break the data producers but instead I'm hoping the changes we are planning are somehow backward compatible. In any case, I'm good with putting it in beta as Elastic Agent is still in beta.

@ebeahan Anything I need to do on my end for the above?

@ebeahan ebeahan changed the title Stage 3 changes for RFC 0009 - data_stream fields Stage 2 changes for RFC 0009 - data_stream fields Apr 19, 2021
@ebeahan ebeahan force-pushed the rfc/0009/apply-stage-3-changes branch from 885d5a7 to 8cf6197 Compare April 19, 2021 15:24
@ebeahan
Copy link
Member Author

ebeahan commented Apr 19, 2021

I've updated the changes here to include the beta header on the fieldset (preview).

@ruflin Can you perform a final review here as well as on #1367?

Copy link
Contributor

@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.

Only reviewed the beta stage again. Thanks @ebeahan for pushing this forward.

@ebeahan ebeahan merged commit 5cfac46 into elastic:master Apr 19, 2021
@ebeahan ebeahan deleted the rfc/0009/apply-stage-3-changes branch April 19, 2021 18:14
ebeahan added a commit to ebeahan/ecs that referenced this pull request Apr 19, 2021
# Conflicts:
#	experimental/generated/elasticsearch/template.json
#	generated/csv/fields.csv
#	generated/elasticsearch/template.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants