-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Convert Filebeat's traefik.access to ECS. #9005
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably reviewed a bit early, still leave the comments in. No need to comment on the ones you going to tackle anyways.
8a96c77
to
5e82eff
Compare
a5d09e1
to
e17ba12
Compare
@ruflin Just got back to this one and finished it. Will be the first decent test run it gets, but I don't expect anything weird this time. Been testing it pretty thoroughly locally (now that I understand better full testsuite). If you could review as well, that would be great. I'd like to merge this one tomorrow too, and pick up the next PRs with nothing left holding me back ;-) |
filebeat/module/traefik/_meta/kibana/6/dashboard/Filebeat-traefik-overview.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field changes LGTM but changes to dashboards should be reverted.
Indeed, that was my very first PR. I will revert this. |
fa384a1
to
3cfc74f
Compare
@ruflin Renames in Kibana objects and ML are gone now. Should be good for a final review. I also introduced a few unrelated improvements in ecs-migration.yml here: I had forgotten mentions of |
Only failure is unrelated. Processor test for field rename, AFAICT: https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-darwin/2020/ |
jenkins test this |
801a07f
to
7145f97
Compare
This was the very first ECS transition PR I wrote, and it had a lot of nasty history. So the latest push is a full rewrite of the PR on the latest master. This warrants a full review again. This is also the first access log I touch after ECS Beta 2. As such, it goes a bit farther than the previous access log migrations in migrating to ECS. Finally, there are 3 new caveats. Things that are missing from this PR, but that I think should be done as follow-up PRs, as they are blocked by various dependencies. |
jenkins, test this |
Failure in Jenkins Filebeat build is caused by the Seems like the attempt to raise the limit (#9613) is not working. The error still shows the 75/5m default... Issue to fix is #9587. Full paste of the error, for posterity:
|
jenkins, test this |
"user_agent.original": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36", | ||
"user_agent.os.full_name": "Linux", | ||
"user_agent.os.name": "Linux", | ||
"user_agent.os.version": "..", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is what the second caveat is about. Right now the UA parser gives us major/minor/patch most of the time, and doesn't give us a full version string.
So I'm setting user_agent.version
and user_agent.os.version
(here and here respectively) with a trivial interpolation. To actually do this right, ideally IN would give this back to us as a full version string (may even be better wrt test versions like betas), second best approach would be to put together a small reuseable pipeline that rebuilds the version string based on these 3-4 fields that may or may not be there, and leverage that everywhere at once.
And the worst but simplest approach was simply to interpolate directly there. Even if I guard it with an if on the presence of .major
, we'll have broken-looking version numbers whenever another one of the numbers is missing (like here). So I didn't want to waste a bunch of time on this.
Another alternative for this PR is simply to address this later, and leave major/minor/patch in place for now, like all of the other access log modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM to delay/skip this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor comment.
++ for the caveats and skipping them for now.
GitHub lost my merge commit and went with the issue definition again 🤦♂️ |
Caveats:
traefik.access.remote_ip
is not renamed. If it's an IP, it's copied tosource.ip
,otherwise copied to
source.domain
.traefik.access.remote_ip
.user_agent.version
anduser_agent.os.version
😂. Shall we postpone this? I'd like to get the bulk of ECS translations done and merged quick. Then I'd like to figure out some of the tricker common adjustments later, and apply en masse.body_sent.bytes
TODO:
user_agent
field settraefik/access/_meta/fields.yml
:long
.address
fields for ambiguous address instead oftraefik.access.remote_ip
user_agent.version