-
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
[Metricbeat] Update HAProxy module to follow ECS #10558
Conversation
@@ -57,7 +56,6 @@ | |||
"server": { | |||
"id": 0 | |||
}, | |||
"service_name": "FRONTEND", |
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.
@webmat I renamed it to service_name
.
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.
I don't agree with this mapping.
In HAProxy you define frontends which map 1:1 with a vhost/ssl configuration. Then you map this frontend to one or multiple backends, like your main pool of app server, and perhaps a fallback.
So frontend is an internal concept to HAProxy, and one HAProxy process can service many of them. So haproxy.service_name is not at all a process.name.
I would have disagreed less with mapping it to service.name, but I still don't think this would have been a good idea.
Actually I was hoping to look into adding support for various kinds of proxies to ECS. The concept of public entrypoints mapping to backends is generic enough to warrant inclusion in ECS.
I think we should revert this. I can take care of it if you agree.
Should the |
Should the |
Should the [EDIT] Or should |
I noticed you are currently changing/aliasing |
@ycombinator Seems like we missed the above ones during reviewing all the fields. For this PR, is there anything in this PR which should not happen? If not, I suggest to merge it and address the other things in a follow up PR to not delay this one. |
@ruflin The only one in this PR that I'd check is #10558 (comment):
Because it could lead to a conflict later. |
These are 2 different metrisets, so both can be mapped to it. Or do I miss something? |
I didn't realize this was possible, but of course, it makes sense! Sorry! |
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.
Follow up PR created here: #10568 I only address the process field as it's the only one mapping to ECS and is why I think it's most important. |
@ycombinator To answer your question about whether to switch I would say no change is necessary here, as the field name is extremely specific to HAProxy. The ECS guideline is more so that generically named fields are able to map to systems with different IDs and codes. But when it's specific to one system, I think chances of IDs moving from numeric to alphanum is really low. When it happens, it will be obvious to everyone it needs an immediate breaking change. Until then, I'd rather avoid this small breaking change. |
No description provided.