Skip to content

[Metricbeat] Update HAProxy module to follow ECS#10558

Merged
ruflin merged 3 commits intoelastic:masterfrom
ruflin:haproxy-ecs
Feb 5, 2019
Merged

[Metricbeat] Update HAProxy module to follow ECS#10558
ruflin merged 3 commits intoelastic:masterfrom
ruflin:haproxy-ecs

Conversation

@ruflin
Copy link
Copy Markdown
Contributor

@ruflin ruflin commented Feb 5, 2019

No description provided.

@ruflin ruflin requested a review from webmat February 5, 2019 09:20
@ruflin ruflin requested a review from a team as a code owner February 5, 2019 09:20
houndci-bot
houndci-bot reviewed Feb 5, 2019
View reviewed changes
ruflin
ruflin commented Feb 5, 2019
View reviewed changes
metricbeat/module/haproxy/stat/_meta/data.json Outdated
Copy link
Copy Markdown
Contributor Author

@ruflin ruflin Feb 5, 2019

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.

Copy link
Copy Markdown
Contributor

@webmat webmat Feb 5, 2019

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.

@ruflin ruflin requested a review from a team as a code owner February 5, 2019 09:23
@ruflin ruflin mentioned this pull request Feb 5, 2019
Closed
@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Feb 5, 2019

Should the haproxy.stat.check.code field be mapped as keyword (currently mapped as a long) per https://github.com/elastic/ecs#ids-and-most-codes-are-keywords-not-integers?

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Feb 5, 2019

Should the haproxy.stat.agent.last field be mapped as keyword (currently mapped as a integer) per https://github.com/elastic/ecs#ids-and-most-codes-are-keywords-not-integers? Note that the haproxy.stat.health.last field is mapped as keyword.

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Feb 5, 2019 •
edited
Loading

Should the haproxy.stat.server.id field be changed/aliased to service.id?

[EDIT] Or should haproxy.stat.proxy.id field be changed/aliased to service.id?

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Feb 5, 2019

I noticed you are currently changing/aliasing haproxy.stat.process_id to process.pid. What about haproxy.info.pid?

@ruflin
Copy link
Copy Markdown
Contributor Author

ruflin commented Feb 5, 2019

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

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Feb 5, 2019

For this PR, is there anything in this PR which should not happen?

@ruflin The only one in this PR that I'd check is #10558 (comment):

I noticed you are currently changing/aliasing haproxy.stat.process_id to process.pid. What about haproxy.info.pid?

Because it could lead to a conflict later.

@ruflin
Copy link
Copy Markdown
Contributor Author

ruflin commented Feb 5, 2019

These are 2 different metrisets, so both can be mapped to it. Or do I miss something?

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Feb 5, 2019

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!

ycombinator
ycombinator approved these changes Feb 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@ruflin ruflin merged commit c22a977 into elastic:master Feb 5, 2019
@ruflin ruflin deleted the haproxy-ecs branch February 5, 2019 12:41
@ruflin
Copy link
Copy Markdown
Contributor Author

ruflin commented Feb 5, 2019

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.

@webmat
Copy link
Copy Markdown
Contributor

webmat commented Feb 5, 2019

@ycombinator To answer your question about whether to switch haproxy.stat.check.code to keyword, I love that you caught that :-)

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.

@webmat webmat mentioned this pull request Feb 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ycombinator ycombinator ycombinator approved these changes

@webmat webmat Awaiting requested review from webmat

+1 more reviewer

@houndci-bot houndci-bot houndci-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ecs Metricbeat Metricbeat module review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ruflin @ycombinator @webmat @houndci-bot