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

Remove excessive istio attributes to avoid hitting the dimensions limit #765

Merged
merged 3 commits into from
May 3, 2023
Merged

Remove excessive istio attributes to avoid hitting the dimensions limit #765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d609378
Remove excessive istio attributes to avoid hitting the dimensions limit
dmitryax May 2, 2023
b751ff3
Merge branch 'main' into drop-excessive-istio-attributes
dmitryax May 3, 2023
3728c1d
Update helm-charts/splunk-otel-collector/templates/config/_otel-agent…
dmitryax May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
skipped when we use both `multilineConfig` AND `extraOperators` in values.yaml
- Enable retry mechanism in filelog receiver to avoid dropping logs on backpressure from the downstream
pipeline components [#764](https://github.com/signalfx/splunk-otel-collector-chart/pull/764)
- Drop excessive istio attributes to avoid running into the dimensions limit when scraping istio metrics is enabled [765](https://github.com/signalfx/splunk-otel-collector-chart/pull/765)

## [0.75.0] - 2023-04-17

Expand Down
27 changes: 27 additions & 0 deletions helm-charts/splunk-otel-collector/templates/config/_otel-agent.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,30 @@ processors:
new_name: container.memory.usage
{{- end }}

{{- if or .Values.autodetect.prometheus .Values.autodetect.istio }}
# This processor is used to remove excessive istio attributes to avoid running into the dimensions limit.
# This configuration assumes single cluster istio deployment. If you run istio in multi-cluster scenarios or make use of the canonical service and revision labels,
# you may need to adjust this configuration.
attributes/istio:
include:
match_type: regexp
metric_names:
- istio_.*
actions:
jinja2 marked this conversation as resolved.
Show resolved Hide resolved
- action: delete
key: source_cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to add a new value and generate each action so it's more flexible as additional attributes are (un)wanted?

Copy link
Contributor Author

@dmitryax dmitryax May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean having another option in values.yaml for that?

I don't think we should add more configuration options to the already bloated values.yaml for this niche use case.

Overriding the set of attributes is already possible, just a bit more verbose, like:

agent:
  config:
    processors:
      attributes/istio:
        actions:
          - action: delete
            key: another_istio_attribute
          ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the current interface to enable istio metrics is pretty unflexible.

autodetect:
  istio: true

We can probably rethink it to be able to add this list of attributes. Maybe as a follow-up effort. Let me know if you have any ideas

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it would be great if we made this a bit more flexible. Maybe something like this?

autodetect:
  istio:
    enabled: true
    # Explain why we remove these default set of attributes
    excludeAttributes:
    - attr1
    - attr2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is the transition from bool value to the map in a backward-compatible way. We can try something out but I would rather do it as a follow-up effort not blocking the release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed this #769

- action: delete
key: destination_cluster
dmitryax marked this conversation as resolved.
Show resolved Hide resolved
- action: delete
key: source_canonical_service
dmitryax marked this conversation as resolved.
Show resolved Hide resolved
- action: delete
key: destination_canonical_service
- action: delete
key: source_canonical_revision
- action: delete
key: destination_canonical_revision
{{- end }}

# By default only SAPM exporter enabled. It will be pointed to collector deployment if enabled,
# Otherwise it's pointed directly to signalfx backend based on the values provided in signalfx setting.
# These values should not be specified manually and will be set in the templates.
Expand Down Expand Up @@ -774,6 +798,9 @@ service:
processors:
- memory_limiter
- batch
{{- if or .Values.autodetect.prometheus .Values.autodetect.istio }}
- attributes/istio
{{- end }}
- resourcedetection
- resource
{{- if (and .Values.splunkPlatform.metricsEnabled .Values.environment) }}
Expand Down