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

Expose wider collector configuration #31

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Conversation

acrmp
Copy link
Member

@acrmp acrmp commented Jul 12, 2024

⚠️ This is a breaking change to the properties for the collector ⚠️

  • Operators will need to should update the properties to match the format expected and define pipelines.
  • It should be called out in the release notes.

  • Replaces the existing metric_exporters and trace_exporters properties with the new config property.
  • Operators can now provide pipeline and processor configuration.
  • The default otlp receiver will be set on all pipelines.
  • The config can be validated with the following command:
/var/vcap/packages/otel-collector/otel-collector validate --config some-config.yml

The example config provided in the PR refers to the batch processor that is not currently included in the CF distribution of the collector. We'll make a separate PR to include that processor.

ingress.grpc.port:
description: "Port the collector is listening on to receive OTLP over gRPC"
default: 9100
config:

Choose a reason for hiding this comment

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

Hi @acrmp,

Generally I'm all in about this change, but I wonder if we should do it this way, that we practically embed the Otel Collector config in the spec or we provide provide a parameter which can define the path to the Otel collector config yaml file.

If we use some manager to manager the fleet of Otel collectors which we have based on OpAMP, they can push configuration to the clients (Otel collectors), it might be more clear, readable, practical for further processing (merge of local and remote config) if the configuration is in a separate file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@chombium Can you expand on how you would see this working? Were you thinking of doing config merging outside of this release?

We're still exploring how we might want to distribute configuration. One option is that we could do something similar to what we do for the prom scraper and load configs that match a certain file name glob. A downside is that otel config changes would currently require BOSH to roll all the VMs.

We've looked into OpAMP a little. There still seem to be a few rough edges at the moment. For example see this recent issue we opened:
open-telemetry/opentelemetry-collector-contrib#33799

Choose a reason for hiding this comment

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

Hi @acrmp, @mkocher sorry for the late reply.... :-/

I was thinking of having the Otel collector config in a separate file for better separation, but it doesn't hurt if it's inside the job spec. At the end if we use an OpAMP server for managing the configuration, the sever would ping the Otel Collector that there is new config available and its up to the collector to decide when it will download and apply the new config. I'm not against your solution at all ;)

@acrmp
Copy link
Member Author

acrmp commented Jul 16, 2024

We think it's not too onerous to maintain backwards compatibility here and continue to support the older properties for a period. We'll likely push some further changes for this tomorrow.

- Replaces the existing `metric_exporters` and `trace_exporters`
  properties with the new `config` property.
- Operators can now provide pipeline and processor configuration.
- The default otlp receiver will be set on all pipelines.
- The config can be validated with the following command:
  `/var/vcap/packages/otel-collector/otel-collector validate --config some-config.yml`

Signed-off-by: Ausaf Ahmed <[email protected]>
- Continue to support the earlier properties to provide a migration
  path.
- Operators should prefer the new `config` property going forwards.

Signed-off-by: Matthew Kocher <[email protected]>
@mkocher mkocher merged commit f99a107 into main Jul 17, 2024
5 checks passed
@mkocher mkocher deleted the complete-collector-config branch July 17, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants