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

[opampextension]: Move custom message interface to separate module #32951

Merged
merged 16 commits into from May 10, 2024

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented May 8, 2024

Description:

  • Breaks our the custom message interface to a separate module, so other components can use the interface without needing to import the opampextension module in its entirety.

We could temporarily alias the old methods if we'd like, but I think that the CustomMessage stuff has been so short lived that, in addition to the alpha status of the opampextension component, it feels justified to just skip the deprecation process and move it to a new module.

Link to tracking Issue: Closes #32950

Testing:

  • Covered by existing unit tests

Documentation:

  • Added more documentation on usage in the new module.
  • Modified opampextension docs to point to the new module.

@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review May 8, 2024 20:14
@BinaryFissionGames BinaryFissionGames requested a review from a team as a code owner May 8, 2024 20:14
@evan-bradley
Copy link
Contributor

@BinaryFissionGames Could we put this in pkg and make it just a module instead of an extension? Usually extensions are supposed to export a component that gets put into the service, as where modules in the pkg directory are only intended to be used as libraries.

@BinaryFissionGames
Copy link
Contributor Author

BinaryFissionGames commented May 9, 2024

@BinaryFissionGames Could we put this in pkg and make it just a module instead of an extension? Usually extensions are supposed to export a component that gets put into the service, as where modules in the pkg directory are only intended to be used as libraries.

I'm certainly open to it. The reason I chose not to is based on the storage module:
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/storage

It also mirrors in the non-contrib repo how the storage package is also nested under extension:
https://github.com/open-telemetry/opentelemetry-collector/tree/main/extension/experimental/storage

And also the auth module, which contains the authcation extension interfaces:
https://github.com/open-telemetry/opentelemetry-collector/tree/main/extension/auth

So it seemed like this was the current pattern.

Still happy to change it if you think I should.

@BinaryFissionGames
Copy link
Contributor Author

Encoding seems like another example of a module that is not component that's nested under extension:
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/encoding

@evan-bradley
Copy link
Contributor

Thanks for the examples. I haven't dug into those extensions too much, I didn't know they exported interfaces as well. I'd say you have it right putting it under extension, let's leave it as-is.

As a side note, thanks for being mindful of the deprecation process; I agree that we can just move the interfaces as part of a breaking change like you've done here.

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@evan-bradley
Copy link
Contributor

You'll need to add this module to cmd/checkapi/allowlist.txt since we're exporting more than just a NewFactory function.

@evan-bradley evan-bradley merged commit cde2b0a into open-telemetry:main May 10, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 10, 2024
jlg-io pushed a commit to jlg-io/opentelemetry-collector-contrib that referenced this pull request May 14, 2024
…pen-telemetry#32951)

**Description:** <Describe what has changed.>
* Breaks our the custom message interface to a separate module, so other
components can use the interface without needing to import the
`opampextension` module in its entirety.

We could temporarily alias the old methods if we'd like, but I think
that the CustomMessage stuff has been so short lived that, in addition
to the alpha status of the opampextension component, it feels justified
to just skip the deprecation process and move it to a new module.

**Link to tracking Issue:** Closes open-telemetry#32950

**Testing:**
* Covered by existing unit tests

**Documentation:**
* Added more documentation on usage in the new module.
* Modified opampextension docs to point to the new module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[extension/opamp]: Move custom message related interfaces to separate module
3 participants