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

Rename filebeat apache2 module to apache #9402

Merged
merged 16 commits into from
Jan 12, 2019

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Dec 5, 2018

The modules for Apache HTTP Server are called apache in metricbeat
and apache2 in filebeat. Use the same name for both modules.

Fixes #9395.

@jsoriano jsoriano self-assigned this Dec 5, 2018
@jsoriano jsoriano force-pushed the filebeat-apache2-rename branch 2 times, most recently from 34beeb1 to 4081e91 Compare December 5, 2018 15:25
ruflin
ruflin previously requested changes Dec 5, 2018
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.

Before merging this we need to think through on what our expected upgrade path here is. What happens if users upgrade from 6.x to 7.x.

@@ -142,31 +142,31 @@

## Apache

- from: apache2.access.user_name
- from: apache.access.user_name
Copy link
Contributor

Choose a reason for hiding this comment

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

So this becomes now interesting. This is for compatibility with 6.x. In 6.x it will stay apache2. Now comes the tricky part: We will have a mix of apache2 and apache fields. What should we do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not only the fields, but also configurations referring to both names should work during at least some versions (6.7?).
Shall we duplicate the module during some versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ruflin easier, I have reverted the changes in the ECS migration, and I have added aliases for the rest of fields, so we use the same strategy than for ECS.

Copy link
Member Author

Choose a reason for hiding this comment

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

For config migration I am still not sure how to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ruflin I am trying the approach of adding an option to module.yml to indicate that a module has been moved, this keeps old configurations working, see 9af3f75.
If you are ok with this approach I'll open an specific PR just for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR for moved modules #9432

@jsoriano jsoriano added the Team:Integrations Label for the Integrations team label Dec 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/infrastructure

@jsoriano jsoriano force-pushed the filebeat-apache2-rename branch from a2b0ef4 to be2f162 Compare December 6, 2018 15:32
@jsoriano jsoriano added the in progress Pull request is currently in progress. label Dec 6, 2018
jsoriano added a commit to jsoriano/beats that referenced this pull request Dec 7, 2018
If a module needs to be moved, existing configuration will stop working,
this change adds an option to mark an old module as moved to other name.

This is being considered for the movement of apache2 module to apache to
be coherent with metricbeat module (elastic#9402).
@@ -26,6 +26,7 @@ import (
"strings"
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes on this file if done will be done in #9432.

@ruflin
Copy link
Contributor

ruflin commented Dec 7, 2018

I had a chat with @jsoriano about the migration from 6.x to 7.x and it should be working the same ways as for any other module based on the proposal here.

@ruflin ruflin dismissed their stale review December 7, 2018 11:40

Migration should work as expected.

@ruflin
Copy link
Contributor

ruflin commented Dec 7, 2018

PR #8963 should be merged first to reduce the surface of this change.

jsoriano added a commit that referenced this pull request Dec 21, 2018
If a module needs to be moved, existing configuration will stop working,
this change adds an option to mark an old module as moved to other name.

This is being considered for the movement of apache2 module to apache to
be coherent with metricbeat module (#9402).
@jsoriano jsoriano requested review from a team as code owners January 8, 2019 17:42
@urso urso removed the request for review from a team January 8, 2019 22:20
@jsoriano jsoriano force-pushed the filebeat-apache2-rename branch from 8ea885d to 6cc43a1 Compare January 9, 2019 12:03
@jsoriano
Copy link
Member Author

jsoriano commented Jan 9, 2019

@ruflin this would be mostly ready, my main doubt is what to do with dashboards, not sure if I should rename fields there.
How are we going to support deployments with different versions of filebeat? I guess the same will happen with ECS. If filebeat 6.X and 7.X are used in the same deployment, the same dashboard will need to access different fields.
Do dashboards somehow support aliases?

@webmat
Copy link
Contributor

webmat commented Jan 9, 2019

@jsoriano Yes, aggregations support aliases, and all visualizations are built on that. The only part that aliases don't help us with, in the Kibana Objects, is the saved searches. The saved search columns are _source field names. Aliases don't apply to _source at all.

@ruflin
Copy link
Contributor

ruflin commented Jan 10, 2019

What we currently have is that if someone upgrades from 6.x to 7.x, he keeps his old dashboards from 6.x and enables the migration layer. As we have added all alias for the apache2 fields, things should keep working as is.

For dashboards etc. I'm in the progress of writing a migration scripts based on the ecs-migration.yml file. I just realised now that this is missing in this PR and should be added to make sure we rename the fields correctly. Potentially it just needs update to point to the "apache" instead of "apache2" fields.

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.

LGTM. I suggest we merge this and do a ECS cleanup again afterwards.

@webmat Let us know if you have some objections.

description: >
Contains fields for the Apache HTTP Server access logs.
fields:
- name: remote_ip
Copy link
Contributor

Choose a reason for hiding this comment

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

@webmat We should follow up on this and convert it to ECS.

Copy link
Contributor

@webmat webmat Jan 11, 2019

Choose a reason for hiding this comment

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

Yes, I have a note at the end of the Fb modules list (in #8655) to convert to using .address in all remaining places (it was a beta 2 addition, and came from the first migrations, like this one)

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM, and agree with a round of ECS cleanup afterwards

@jsoriano
Copy link
Member Author

Ok, I am merging this, and lets polish when all pieces are in place. Thanks for the feedback and reviews!

@jsoriano jsoriano merged commit 1ce20cb into elastic:master Jan 12, 2019
@jsoriano jsoriano deleted the filebeat-apache2-rename branch January 12, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Filebeat Filebeat in progress Pull request is currently in progress. module Team:Integrations Label for the Integrations team v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants