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.

Sign up for GitHub

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

Jump to bottom

Make the host info metric generation optional #482

Merged
merged 8 commits into from
Jun 13, 2024
Merged

Make the host info metric generation optional #482

merged 8 commits into from
Jun 13, 2024

Conversation

petewall
Copy link
Collaborator

Making this optional, because the application observability plugin in Grafana Cloud will bill for these metrics.

@petewall petewall requested a review from jcreixell April 19, 2024 22:28
@petewall petewall self-assigned this Apr 19, 2024
@petewall petewall linked an issue Apr 19, 2024 that may be closed by this pull request
Make otelcol.connector.host_info optional #478
Closed
@petewall petewall marked this pull request as draft April 19, 2024 22:28
@petewall
Copy link
Collaborator Author

Converting to draft. I want to create an "Application observability" example that:

  1. Enables traces with this feature turned on
  2. Enables profiles and pyroscope
  3. Describes gathering data from Beyla

@petewall
Copy link
Collaborator Author

Also, what do we do about AppO11y customers that were using this previously, then upgrade? They'll lose AppO11y features, making this a potentially breaking change.

petewall
petewall commented Apr 19, 2024
View reviewed changes
charts/k8s-monitoring/values.schema.json Outdated
Comment on lines 1300 to 1317
},
"alloy": {
"properties": {
"metricsTuning": {
"properties": {
"excludeMetrics": {
"items": {
"type": "string"
}
},
"includeMetrics": {
"items": {
"type": "string"
}
}
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was apparently a duplicate alloy section

@jcreixell
Copy link
Contributor

Also, what do we do about AppO11y customers that were using this previously, then upgrade? They'll lose AppO11y features, making this a potentially breaking change.

I'd propose the following:

  1. Set the default value of the new flag to true
  2. Rollout plugin changes that allow disabling this functionality
  3. Set the default value to false in the chart (with the plugin still defaulting to true)

WDYT?

@petewall petewall force-pushed the feat/optional-otelcol.connector.host_info branch from 451d084 to 4582e2e Compare June 9, 2024 14:05
@petewall petewall marked this pull request as ready for review June 9, 2024 14:35
@petewall petewall requested review from rlankfo, jcreixell, a team, skl, nevermind89x and jewbetcha and removed request for a team June 9, 2024 14:42
@petewall
Copy link
Collaborator Author

petewall commented Jun 9, 2024

Ready to review, please take a look and let's get the process going.
When this is merged and released, I'll do an associated change to make this disabled in the plugin.

jcreixell
jcreixell reviewed Jun 10, 2024
View reviewed changes
Copy link
Contributor

@jcreixell jcreixell left a comment

Choose a reason for hiding this comment

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

overall it LGTM. However, I think it would be idea to try to disassociate the grafanaCloudConnector from traces (as in traces.grafanaCloudMetrics.enabled), since we expect to generate this metric from other signals types in the future

@petewall petewall requested a review from jcreixell June 12, 2024 18:13
jcreixell
jcreixell approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@jcreixell jcreixell left a comment

Choose a reason for hiding this comment

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

2 small nits, otherwise LGTM!

@petewall petewall force-pushed the feat/optional-otelcol.connector.host_info branch from 1935759 to b59fc7a Compare June 13, 2024 16:17
@petewall petewall merged commit 52ba8fc into main Jun 13, 2024
16 checks passed
@petewall petewall deleted the feat/optional-otelcol.connector.host_info branch June 13, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcreixell jcreixell jcreixell approved these changes

@skl skl skl approved these changes

@rlankfo rlankfo Awaiting requested review from rlankfo

@nevermind89x nevermind89x Awaiting requested review from nevermind89x nevermind89x was automatically assigned from grafana/k8s-monitoring-dev

@jewbetcha jewbetcha Awaiting requested review from jewbetcha jewbetcha was automatically assigned from grafana/k8s-monitoring-dev

Assignees

@petewall petewall

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make otelcol.connector.host_info optional
4 participants
@petewall @jcreixell @rlankfo @skl