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

[receivers/hostmetrics] add conntrack metrics #11769

Merged
merged 3 commits into from
Jul 20, 2022

Conversation

codeboten
Copy link
Contributor

For linux only, adding metrics about connection tracking table.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 12, 2022
@codeboten codeboten removed the Stale label Jul 15, 2022
@codeboten codeboten force-pushed the codeboten/conntrack branch from ccd0d36 to f0b72b5 Compare July 15, 2022 15:26
@codeboten codeboten marked this pull request as ready for review July 15, 2022 15:26
@codeboten codeboten requested a review from a team July 15, 2022 15:26
@codeboten codeboten requested a review from dmitryax as a code owner July 15, 2022 15:26
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitryax
Copy link
Member

Why do we want these metrics to be emitted by default? they seems like a good candidate for optional metrics

@codeboten
Copy link
Contributor Author

Why do we want these metrics to be emitted by default? they seems like a good candidate for optional metrics

What's the best way to determine when a metric should be on by default vs not?

@dmitryax
Copy link
Member

We don't have any strict guidelines to determine that, probably we should have it. Usually it's just up to code owner.

I usually follow the following reasoning:

  • The default set of metrics is enough to monitor a system at whole to determine its healthiness.
  • Optional metrics are additional nice-to-have metrics that are not essential to determine system healthiness.

We also should take into consideration how stable a receiver is. If it has been used in production for some time, adding default metrics can affect downsteam backends that will unexpectedly receive additional load caused by added metrics with another collector upgrade.

For linux only, adding metrics about connection tracking table. These are disabled by default.
@codeboten codeboten force-pushed the codeboten/conntrack branch from 2ccad0b to 863052e Compare July 19, 2022 21:41
@codeboten
Copy link
Contributor Author

Why do we want these metrics to be emitted by default? they seems like a good candidate for optional metrics

I agree, have disabled them by default. PTAL

@TylerHelmuth
Copy link
Member

We don't have any strict guidelines to determine that, probably we should have it. Usually it's just up to code owner.

I usually follow the following reasoning:

  • The default set of metrics is enough to monitor a system at whole to determine its healthiness.
  • Optional metrics are additional nice-to-have metrics that are not essential to determine system healthiness.

We also should take into consideration how stable a receiver is. If it has been used in production for some time, adding default metrics can affect downsteam backends that will unexpectedly receive additional load caused by added metrics with another collector upgrade.

These thoughts would be awesome in the "Adding metrics to existing receivers" section of the contributing guide 👀

@dmitryax dmitryax added the ready to merge Code review completed; ready to merge by maintainers label Jul 19, 2022
@codeboten codeboten merged commit 960b841 into open-telemetry:main Jul 20, 2022
@codeboten codeboten deleted the codeboten/conntrack branch July 20, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants