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

feat(metrics): add ability to use OTLP source #2949

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

swiatekm
Copy link

@swiatekm swiatekm commented Mar 21, 2023

Add a setting for switching to the OTLP source for metrics.

The source is created for all users with setup enabled, and the setting only controls if we're sending data to it. It's intentionally one source vs the 8 with have for HTTP, as I believe the technical reasons for the latter are no longer valid.

In terms of implementation, parts of the metrics pipeline become conditional.

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 swiatekm force-pushed the feat/metrics/otlp-source branch 2 times, most recently from d7bb601 to fcb9561 Compare March 21, 2023 16:07
@github-actions github-actions bot added the documentation documentation label Mar 21, 2023
@swiatekm swiatekm force-pushed the feat/metrics/otlp-source branch from fcb9561 to f07a1b4 Compare March 23, 2023 13:06
@swiatekm swiatekm marked this pull request as ready for review March 23, 2023 13:06
@swiatekm swiatekm requested a review from a team as a code owner March 23, 2023 13:06
Comment on lines 343 to 353
exporters:
- sumologic/default
{{- if eq .Values.sumologic.metrics.sourceType "http" }}
- sumologic/apiserver
- sumologic/control_plane
- sumologic/controller
- sumologic/kubelet
- sumologic/node
- sumologic/scheduler
- sumologic/state
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Will that work fine (without any blacklisting and throttling due to metrics cardinality)?

Copy link
Author

Choose a reason for hiding this comment

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

I believe so. And if not, I think I'd rather put a hold on this PR instead of reproducing the 8 source setup in otlp. But from what I know, we're either there, or will be very soon.

Copy link
Author

Choose a reason for hiding this comment

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

In the meantime, can you review these changes on the assumption that this ok to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, remaining changes looks fine for me

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains invalid labels. Please remove all of the following labels: ['do-not-merge/hold']

@swiatekm swiatekm force-pushed the feat/metrics/otlp-source branch 2 times, most recently from 611ace8 to 3140422 Compare June 2, 2023 16:27
@swiatekm swiatekm force-pushed the feat/metrics/otlp-source branch 4 times, most recently from ee95d73 to 2693b9b Compare July 13, 2023 15:47
@swiatekm swiatekm force-pushed the feat/metrics/otlp-source branch 6 times, most recently from b42763e to a2df05d Compare August 21, 2023 10:25
@swiatekm swiatekm merged commit 55b7ab0 into main Sep 6, 2023
@swiatekm swiatekm deleted the feat/metrics/otlp-source branch September 6, 2023 16:29
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.

3 participants