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

[extension/storage/dbstorage] Create component #7061

Merged
merged 8 commits into from
Jan 12, 2022

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jan 6, 2022

Description:
Add an experimental dbstorage extension, similar to filestorage.

Supports Postgres and sqlite.

Sponsor: @dmitryax

  • A complete config.go, which will show how your component will look like to users
  • README.md with some basic information about your component
  • versions.yaml
  • CODEOWNERS with at least the sponsor and the component's author
  • internal/components tests
  • go.mod at the root of the repository and cmd/configschema
  • dependabot
  • The skeleton of the component, potentially using the helpers
  • Makefile

Testing:
Unit tests.

Documentation:
README added

Fixes #7114

@atoulme atoulme requested a review from a team as a code owner January 6, 2022 01:44
@atoulme atoulme force-pushed the dbstorage branch 3 times, most recently from 4516e71 to 95ca006 Compare January 6, 2022 18:27
@dmitryax
Copy link
Member

dmitryax commented Jan 7, 2022

Hey @atoulme , we are in the process of introducing new guidelines for adding new component. Please check the this issue and add the checklist to this PR or create an issue.

You can add me as sponsor.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

One nit. Otherwise LGTM

extension/storage/dbstorage/README.md Outdated Show resolved Hide resolved
@atoulme atoulme requested a review from dmitryax January 12, 2022 21:29
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM, the clarification about how to extend a distribution to use a new driver can be part of a follow-up PR.


`driver`: the name of the database driver to use. By default, the storage client supports "sqlite3" and "pgx".

Implementors can add additional driver support by importing SQL drivers into the program.
Copy link
Member

Choose a reason for hiding this comment

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

How would it work concretely? I can't think of a clean way of doing that using the builder, and not using it means having to manage more than just an import statement. But perhaps I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I get, folks building their own distros can add imports of additional drivers and import them using the underscore import, just like this SO thread explains: https://stackoverflow.com/questions/21220077/what-does-an-underscore-in-front-of-an-import-statement-mean
As long the init() function of those side-effect imports registers the driver, it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but this import has to be in a source file that is included in the distribution. So, either a downstream distribution creates a whole new extension just for the import statement, or they have to maintain the source code for their distribution (main.go essentially) because of that import line. Or perhaps there's a better solution I'm not seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess they'd add that to their main.go? I just don't know any better, I'm sorry.

Copy link
Member

Choose a reason for hiding this comment

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

Right -- they might just not have a main.go yet. I'm OK with approving and merging this as is, but would still be good to have an end-to-end example for users interested in extending with their SQL drivers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you please file an issue, sorry - especially interested in where such a sample would live?

Copy link
Member

Choose a reason for hiding this comment

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

Done: #7157

@jpkrohling jpkrohling changed the title add dbstorage extension [extension/storage/dbstorage] Create component Jan 12, 2022
@jpkrohling jpkrohling merged commit 772f39c into open-telemetry:main Jan 12, 2022
@atoulme atoulme deleted the dbstorage branch January 12, 2022 22:49
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.

[new component] Database storage
3 participants