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

Clearly separate Config and Settings terms used in struct names throughout the codebase #6767

Closed
3 tasks done
dmitryax opened this issue Dec 13, 2022 · 9 comments
Closed
3 tasks done
Assignees
Labels
good first issue Good for newcomers release:required-for-ga Must be resolved before GA release

Comments

@dmitryax
Copy link
Member

dmitryax commented Dec 13, 2022

Currently, we use the Config term for collector configuration as a whole and parts of it defined for receivers/processors/exporters/extensions. Examples:

  • otelcol.Config
  • component.Config
  • otlpexporter.Config

But for unmarshalling some deeper parts of the user config, we use Settings structs, for example:

  • exporterhelper.QueueSettings
  • configgrpc.GRPCClientSettings
  • configtls.TLSSetting

At the same time service/telemetry package still uses Config, e.g. MetricsConfig, LogsSamplingConfig, and TracesConfig.

The suggestion is to use Config term for all the structs used to unmarshal any parts of the YAML config provided by users. We will keep Settings only for structs that are populated programmatically, like:

  • component.TelemetrySettings
  • component.ReceiverSettings
@bogdandrutu
Copy link
Member

I think we should start working on this, because we cannot delay until the stability.

@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Jan 25, 2024
dmitryax pushed a commit that referenced this issue Jan 26, 2024
…9392)

**Description:**
Deprecate CORSSettings, use CORSConfig instead

**Link to tracking Issue:**
#6767
dmitryax pushed a commit that referenced this issue Jan 26, 2024
…ad (#9405)

**Description:**
Deprecate HTTPServerSettings, use HTTPServerConfig instead

**Link to tracking Issue:**
#6767
dmitryax pushed a commit that referenced this issue Jan 29, 2024
…ad (#9404)

**Description:**
Deprecate HTTPClientSettings, use HTTPClientConfig instead

**Link to tracking Issue:**
#6767
dmitryax pushed a commit that referenced this issue Feb 1, 2024
…9403)

**Description:**
Deprecate GRPCServerSettings, use ~GRPC~ServerConfig instead

**Link to tracking Issue:**
#6767
dmitryax pushed a commit that referenced this issue Feb 2, 2024
…9402)

**Description:**
Deprecate GRPCClientSettings, use ClientConfig instead

**Link to tracking Issue:**
#6767
dmitryax pushed a commit that referenced this issue Feb 16, 2024
…nfig instead (#9401)

**Description:**
Deprecate ScraperControllerSettings, use ~Scraper~ControllerConfig
instead

**Link to tracking Issue:**
#6767
dmitryax pushed a commit that referenced this issue Feb 23, 2024
**Description:** 
Removes deprecated items from `confighttp` 

**Link to tracking Issue:** Related to
#6767
@TylerHelmuth
Copy link
Member

Removing from the confighttp milestone as the public structs now use Config.

dmitryax pushed a commit that referenced this issue Mar 20, 2024
…ultScraperControllerSettings` (#9783)

**Description:** 
Remove deprecated `ScraperControllerSettings` and
`NewDefaultScraperControllerSettings`

**Link to tracking Issue:**
#6767
@codeboten
Copy link
Contributor

Closing this in favour of #9428

@dmitryax dmitryax reopened this Jun 5, 2024
@codeboten
Copy link
Contributor

exporterhelper still needs updating

@dmitryax dmitryax self-assigned this Jun 5, 2024
@mx-psi mx-psi added the good first issue Good for newcomers label Aug 21, 2024
@mx-psi
Copy link
Member

mx-psi commented Sep 9, 2024

We still need to update:

  • exporterhelper.TimeoutSettings
  • NewDefaultTimeoutSettings
  • exporterhelper.QueueSettings
  • NewDefaultQueueSettings

@jade-guiton-dd
Copy link
Contributor

I'll work on this.

mx-psi pushed a commit that referenced this issue Sep 11, 2024
#### Description

This PR renames `TimeoutSettings` and `QueueSettings` (as well as the
corresponding `NewDefault` functions) to `TimeoutConfig` and
`QueueConfig`, for naming consistency reasons.

The previous struct/function names are kept as deprecated aliases of the
new ones for now.

#### Link to tracking issue

Updates #6767  (edit: reworded by @mx-psi)

#### Testing

No behavior changes were made, so no additional testing should be
necessary.

The references to the aforementioned structs/functions in existing tests
have been renamed as well. This means the deprecated function aliases
are not tested, lowering the coverage a bit.

#### Documentation

The new functions are documented identically to the previous ones.
mx-psi pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Sep 13, 2024
…tings (#35158)

**Description:**

`TimeoutSettings` and `QueueSettings` in the `exporterhelper` core
collector package were renamed to `TimeoutConfig` and `QueueConfig` for
naming consistency reasons, and the related `NewDefault` functions were
renamed as well. This PR updates the core libraries to a prerelease
version with those changes, and updates opentelemetry-collector-contrib
modules to the new names, to plan for the deprecation of the old names
in v0.110.0.

**Link to tracking Issue:**

Related to [this
PR](open-telemetry/opentelemetry-collector#11132)
and [this
issue](open-telemetry/opentelemetry-collector#6767)
on the core collector.

**Notes:**

- `go.opentelemetry.io/collector/cmd/mdatagen` is specifically *not*
updated, as it currently has a bug causing a CI test failure.
- The `prometheus-compliance-tests` CI failure is unrelated to this PR.
mx-psi pushed a commit that referenced this issue Sep 16, 2024
…11178)

#### Description

My PR #11132 introduced a small breaking change in the API of
`otlpexporter`, which I failed to notice at the time. This PR adds a
release note about this.

Specifically, the `TimeoutSettings` field in `otlpexporter.Config` was
renamed to `TimeoutConfig` ([link to the new
code](https://github.com/jade-guiton-dd/opentelemetry-collector/blob/543c4f510d3bbcd50e914f4e5d7f22c5fcbda92d/exporter/otlpexporter/config.go#L23)).
As this is an embedded field, renaming the type of the field renamed the
field itself.

(Another option would have been to de-embed the field, and keep the old
name. This has less potential for breakage, but would technically also
be a breaking change.)

#### Link to tracking issue
Indirectly related to #6767
bogdandrutu pushed a commit that referenced this issue Sep 25, 2024
…11264)

#### Description

Follow-up to #11132 now that 0.110 has been released.

#### Link to tracking issue

Should be the final step for #6767.
@jade-guiton-dd
Copy link
Contributor

If I'm not mistaken, #11264 was the last step for this issue, so it can probably be closed.

@mx-psi
Copy link
Member

mx-psi commented Oct 1, 2024

Taking a look at the output of rg 'type \w*Setting' -tgo -i it looks like this is indeed the case 🚀

@mx-psi mx-psi closed this as completed Oct 1, 2024
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue Oct 4, 2024
…tings (open-telemetry#35158)

**Description:**

`TimeoutSettings` and `QueueSettings` in the `exporterhelper` core
collector package were renamed to `TimeoutConfig` and `QueueConfig` for
naming consistency reasons, and the related `NewDefault` functions were
renamed as well. This PR updates the core libraries to a prerelease
version with those changes, and updates opentelemetry-collector-contrib
modules to the new names, to plan for the deprecation of the old names
in v0.110.0.

**Link to tracking Issue:**

Related to [this
PR](open-telemetry/opentelemetry-collector#11132)
and [this
issue](open-telemetry/opentelemetry-collector#6767)
on the core collector.

**Notes:**

- `go.opentelemetry.io/collector/cmd/mdatagen` is specifically *not*
updated, as it currently has a bug causing a CI test failure.
- The `prometheus-compliance-tests` CI failure is unrelated to this PR.
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this issue Oct 8, 2024
…pen-telemetry#11264)

#### Description

Follow-up to open-telemetry#11132 now that 0.110 has been released.

#### Link to tracking issue

Should be the final step for open-telemetry#6767.
mx-psi added a commit that referenced this issue Oct 29, 2024
…n coding guidelines (#11549)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Reflects existing practice in coding guidelines.

#### Link to tracking issue

Relates to #6767, fixes #9428
lapis-lark pushed a commit to lapis-lark/otel-cassandra-exporter-rework that referenced this issue Nov 4, 2024
…tings (#35158)

**Description:**

`TimeoutSettings` and `QueueSettings` in the `exporterhelper` core
collector package were renamed to `TimeoutConfig` and `QueueConfig` for
naming consistency reasons, and the related `NewDefault` functions were
renamed as well. This PR updates the core libraries to a prerelease
version with those changes, and updates opentelemetry-collector-contrib
modules to the new names, to plan for the deprecation of the old names
in v0.110.0.

**Link to tracking Issue:**

Related to [this
PR](open-telemetry/opentelemetry-collector#11132)
and [this
issue](open-telemetry/opentelemetry-collector#6767)
on the core collector.

**Notes:**

- `go.opentelemetry.io/collector/cmd/mdatagen` is specifically *not*
updated, as it currently has a bug causing a CI test failure.
- The `prometheus-compliance-tests` CI failure is unrelated to this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers release:required-for-ga Must be resolved before GA release
Projects
Archived in project
Development

No branches or pull requests

7 participants