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

fix(metrics): don't forward duplicate metrics from annotated Pods #3171

Merged
merged 1 commit into from Jul 28, 2023

Conversation

swiatekm-sumo
Copy link
Contributor

Don't forward App metrics if they originate from a Pod scraped due to annotations. We tell customers to do this for these metrics (https://help.sumologic.com/docs/integrations/web-servers/nginx/#configure-metrics-collection), but we have separate remote writes for these cases, and so they are forwarded twice, and with different labels.

I ended up making a bunch of additional changes:

  • I've added a integration test for nginx metrics using Telegraf operator
  • Refactored some receiver-mock related code in integration tests
  • Added a utility function to assert metrics are present for a given filter
  • Fixed some minor issues

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

@swiatekm-sumo swiatekm-sumo force-pushed the fix/metric/duplicate-app-metrics branch 4 times, most recently from b31abb6 to d957829 Compare July 27, 2023 15:00
@swiatekm-sumo swiatekm-sumo force-pushed the fix/metric/duplicate-app-metrics branch from d957829 to bbf017a Compare July 27, 2023 16:23
@@ -38,7 +38,7 @@ processors:
metrics:
metric:
# we let the metrics from annotations ("kubernetes-pods") through as they are
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# we let the metrics from annotations ("kubernetes-pods") through as they are
# we let the metrics from annotations ("pod-annotations") through as they are

Comment on lines -416 to -418
internal.ReceiverMockNamespace,
internal.ReceiverMockServiceName,
internal.ReceiverMockServicePort,
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not going to check receiver-mock metrics anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't change any of the test checks, I just moved this logic to a separate function which gets these values from module constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do want to get rid of checking for receiver-mock metrics, because they're annoying to work with - receiver-mock only emits some of them based on ingested data, so they're different for each test - but I didn't do that in this PR.

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

Going to merge it in order to put into the release

@sumo-drosiek sumo-drosiek merged commit 5408da5 into main Jul 28, 2023
40 checks passed
@sumo-drosiek sumo-drosiek deleted the fix/metric/duplicate-app-metrics branch July 28, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants