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

Add missing metrics to fluentd #718

Merged
merged 3 commits into from
Jun 9, 2020
Merged

Conversation

sumo-drosiek
Copy link
Contributor

Description

Adds fluentd_input_status_num_records_total and fluentd_output_status_num_records_total metrics

Fixes #717

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@sumo-drosiek sumo-drosiek added this to the v1.1 milestone Jun 8, 2020
@sumo-drosiek sumo-drosiek changed the title Drosiek fluentd prometheus metrics Add missing metrics to fluentd Jun 8, 2020
@sumo-drosiek
Copy link
Contributor Author

sumo-drosiek commented Jun 8, 2020

I got:

  • ~5% of DPM because of fluentd_input_status_num_records_total
  • ~25% of DPM because of fluentd_output_status_num_records_total

I tested it on empty vagrant environment

Regarding that, I'm going to disable those metrics by default

@frankreno
Copy link
Contributor

The increase makes sense as I assume it is by plugin ID, which adds a good amount of cardinality to the data.

Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

A few comments, other than that plus adding filtering this looks good.

{{- end }}
@include buffer.output.conf
</buffer>
@type copy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we switch to copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type counter
desc The total number of incoming records
<labels>
tag ${tag}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what leads to very high cardinality in the case of logs. Each tag is the full file path for the container log, and it includes the container and pod ID, so it can drastically increase the metrics cardinality here.

@sumo-drosiek sumo-drosiek force-pushed the drosiek-fluentd-prometheus-metrics branch 2 times, most recently from 879cb67 to d9bb74f Compare June 8, 2020 14:05
Copy link
Contributor

@vsinghal13 vsinghal13 left a comment

Choose a reason for hiding this comment

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

LGTM

@perk-sumo perk-sumo force-pushed the drosiek-fluentd-prometheus-metrics branch from 9d1eecd to ebfbc60 Compare June 9, 2020 06:48
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

👍

@perk-sumo perk-sumo merged commit ebfbc60 into master Jun 9, 2020
@perk-sumo perk-sumo deleted the drosiek-fluentd-prometheus-metrics branch June 9, 2020 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing fluentd metrics
4 participants