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

Move sqlquery receiver logic out to a library for reuse #14321

Closed
wants to merge 4 commits into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Sep 20, 2022

Description:
Extract sqlquery receiver logic out to pkg/sqlquery so it may be reused across components.

Link to tracking Issue:
#13547

Testing:
Same unit testing, just moving files.

Documentation:
Simple README.md.

@@ -0,0 +1,49 @@
# SQL Query
Copy link
Member

Choose a reason for hiding this comment

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

This looks a documentation for sqlquery receiver. I'm not sure if is relevant enough to this package to keep it in sync with the receiver.

@dmitryax
Copy link
Member

dmitryax commented Sep 30, 2022

Please rebase

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 15, 2022
@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package sqlqueryreceiver // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/sqlqueryreceiver"
package sqlquery // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sqlquery"
Copy link
Member

@pmcollins pmcollins Oct 17, 2022

Choose a reason for hiding this comment

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

How confident are you that we need to move the receiver config to be part of this library? I feel like the config is a receiver-only thing, and the sqlquery API shouldn't know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to redo this work, based off what I learned from oracledb. This PR is not good enough.

@github-actions github-actions bot removed the Stale label Oct 20, 2022
@atoulme
Copy link
Contributor Author

atoulme commented Nov 3, 2022

As noted, this PR will get a redo once the #16044 PR is merged. Trying to not rush this.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 20, 2022
@atoulme
Copy link
Contributor Author

atoulme commented Dec 29, 2022

I'm just not sure this is a good idea anymore, if only because the sqlqueryreceiver has more dependencies to other DB drivers. Closing.

@atoulme atoulme closed this Dec 29, 2022
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.

3 participants