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

[connector,exporter,processor,receiver] Error out on mismatched type #12381

Merged
merged 12 commits into from
Feb 27, 2025

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Feb 13, 2025

Description

Error out on mismatched type for all component kinds. Same as #12305 but for all component kinds.

Link to tracking issue

Requires #12357
Fixes #12221

@mx-psi mx-psi marked this pull request as ready for review February 14, 2025 12:55
@mx-psi mx-psi requested a review from a team as a code owner February 14, 2025 12:55
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 77.21519% with 18 lines in your changes missing coverage. Please review.

Project coverage is 92.06%. Comparing base (58f3efb) to head (c582257).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
connector/connector.go 33.33% 12 Missing and 6 partials ⚠️

❌ Your patch status has failed because the patch coverage (77.21%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12381      +/-   ##
==========================================
- Coverage   92.16%   92.06%   -0.11%     
==========================================
  Files         465      469       +4     
  Lines       25201    25323     +122     
==========================================
+ Hits        23226    23313      +87     
- Misses       1576     1602      +26     
- Partials      399      408       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Please make sure we have tests for all the main use-cases.

mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Feb 19, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Tries to fix some of the contrib tests for
open-telemetry/opentelemetry-collector/pull/12381

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

Updates open-telemetry/opentelemetry-collector/issues/12221
@mx-psi
Copy link
Member Author

mx-psi commented Feb 19, 2025

@bogdandrutu The remaining contrib failures are interesting. We have quite a few components that use other components under the hood. A couple of examples (but there a few more):

  1. The simple Prometheus receiver uses the Prometheus receiver and has a simplified configuration
  2. The JMX receiver uses an OTLP receiver under the hood

How should these components use the API? I think this is also interesting for telemetry: if a component creates other components under the hood, what should its telemetry look like? cc @djaglowski @jade-guiton-dd in case you have thoughts about this

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Feb 19, 2025

  • Regarding telemetry:

    The component-related attributes defined in the RFC only really work within the context of a standard Collector pipeline (because of otelcol.pipeline.id) and configuration (because of the "config name" part of otelcol.component.id), so I think components that are created manually and used as part of an opaque "sub-pipeline" like that should just inherit the attributes of their containing component. This should happen by default with Wrap TelemetrySettings providers to add component-identifying attributes #12384 as long as the parent component passes the TelemetrySettings they received.

  • Regarding the API:

    I'm not sure if this kind of "sub-pipeline" should be considered a supported use case of components, so I think the onus should be on component authors to manually adjust the settings passed into the child component to make it work, in this case manually adjusting the component ID.

    If we DO want to support this use case, I think the best way would be some kind of API to create a "derived" receiver/exporter/etc.Settings with the appropriate values for the child component, instead of parents passing their own as-is. We could then adjust the ID accordingly. In the context of component telemetry, we could also add additional extra attributes to identify the source as a child component.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Is this unblocked now that #12357 is merged?

@mx-psi
Copy link
Member Author

mx-psi commented Feb 24, 2025

@codeboten I want to fix these first #12381 (comment)

@mx-psi
Copy link
Member Author

mx-psi commented Feb 26, 2025

Filed open-telemetry/opentelemetry-collector-contrib/pull/38221 for fixing the remaining contrib tests.

mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Feb 26, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Fixes some component IDs that did not match the component type.

For components that create other components I chose to go with `<child
Type>/<parent Type>/<parent Name>`

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Needed for open-telemetry/opentelemetry-collector/pull/12381
gantrior pushed a commit to gantrior/solarwinds-otel-collector that referenced this pull request Feb 27, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Tries to fix some of the contrib tests for
open-telemetry/opentelemetry-collector/pull/12381

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

Updates open-telemetry/opentelemetry-collector/issues/12221
@mx-psi mx-psi requested a review from dmathieu as a code owner February 27, 2025 11:59
@mx-psi mx-psi enabled auto-merge February 27, 2025 12:04
@mx-psi mx-psi added this pull request to the merge queue Feb 27, 2025
Merged via the queue into open-telemetry:main with commit c1af501 Feb 27, 2025
51 of 54 checks passed
@mx-psi mx-psi deleted the mx-psi/error-out branch February 27, 2025 12:45
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.

[extension/receiver/exporter/processor/connector] Should we remove component.Type from Settings.ID?
4 participants